From 35e9a9edfe98c7bd5f8ed671bd4d1ddf541c8461 Mon Sep 17 00:00:00 2001 From: AgelxNash Date: Sun, 6 Sep 2026 08:27:58 +0200 Subject: [PATCH 01/19] Integration task: collect all open PRs into one testable branch Task journal: fork created (AgelxNash/MiniShop3), all PR refs fetched, each PR applied as a single squash commit, conflicting PRs skipped and listed below. Our integration changes come first. Merged: 596 598 600 614 619 620 623 624 627 629 631 637 639 642 643 647 648 649. Skipped (conflicts): 599 603 604 605 621 633 638 640 644 646. Drafts excluded. This PR itself (#650) is excluded. From b97a84a4fdcc0f7d2600fb1af09e0bd1e7a9b39c Mon Sep 17 00:00:00 2001 From: Ibochkarev <2138260+Ibochkarev@users.noreply.github.com> Date: Sun, 6 Sep 2026 08:27:29 +0200 Subject: [PATCH 02/19] PR #596: feat(order): lifecycle gate with ports, idempotency, and rollback https://github.com/modx-pro/MiniShop3/pull/596 --- _build/elements/settings.php | 10 + .../minishop3/lexicon/en/default.inc.php | 3 + .../minishop3/lexicon/en/setting.inc.php | 4 + .../minishop3/lexicon/ru/default.inc.php | 3 + .../minishop3/lexicon/ru/setting.inc.php | 4 + .../src/Controllers/Payment/Payment.php | 29 +- .../minishop3/src/ServiceRegistry.php | 25 +- .../src/ServiceRegistryFactories.php | 7 +- .../Order/ManagerOrderMutationService.php | 19 +- .../Order/NullOrderLifecyclePorts.php | 28 ++ .../Order/OrderLifecyclePortsInterface.php | 40 ++ .../src/Services/Order/OrderStatusService.php | 138 +++++-- .../Order/OrderStatusTransitionPolicy.php | 89 ++++ .../Order/OrderStatusServiceLifecycleTest.php | 380 ++++++++++++++++++ .../Order/OrderStatusTransitionPolicyTest.php | 57 +++ 15 files changed, 781 insertions(+), 55 deletions(-) create mode 100644 core/components/minishop3/src/Services/Order/NullOrderLifecyclePorts.php create mode 100644 core/components/minishop3/src/Services/Order/OrderLifecyclePortsInterface.php create mode 100644 core/components/minishop3/src/Services/Order/OrderStatusTransitionPolicy.php create mode 100644 core/components/minishop3/tests/Unit/Services/Order/OrderStatusServiceLifecycleTest.php create mode 100644 core/components/minishop3/tests/Unit/Services/Order/OrderStatusTransitionPolicyTest.php diff --git a/_build/elements/settings.php b/_build/elements/settings.php index c8c371cd6..fa5958b21 100644 --- a/_build/elements/settings.php +++ b/_build/elements/settings.php @@ -296,6 +296,16 @@ 'xtype' => 'numberfield', 'area' => 'ms3_statuses', ], + 'ms3_status_sent' => [ + 'value' => 4, + 'xtype' => 'numberfield', + 'area' => 'ms3_statuses', + ], + 'ms3_order_status_transitions' => [ + 'value' => '', + 'xtype' => 'textfield', + 'area' => 'ms3_statuses', + ], 'ms3_customer_cancel_allowed_statuses' => [ 'value' => '2,3', 'xtype' => 'textfield', diff --git a/core/components/minishop3/lexicon/en/default.inc.php b/core/components/minishop3/lexicon/en/default.inc.php index 163378ff0..d8cea3577 100644 --- a/core/components/minishop3/lexicon/en/default.inc.php +++ b/core/components/minishop3/lexicon/en/default.inc.php @@ -193,6 +193,9 @@ $_lang['ms3_err_status_fixed'] = 'Fixed status is set. You cannot change it to earlier one.'; $_lang['ms3_err_status_wrong'] = 'Invalid order status.'; $_lang['ms3_err_status_same'] = 'This status is already set.'; +$_lang['ms3_err_status_transition'] = 'This status transition is not allowed.'; +$_lang['ms3_err_status_transitions_invalid'] = 'Order status transition allow-list is invalid.'; +$_lang['ms3_err_status_rollback'] = 'Failed to roll back order status after a rejected transition.'; $_lang['ms3_err_register_globals'] = 'Error: php parameter register_globals must be disabled.'; $_lang['ms3_err_link_equal'] = 'You are trying to add product link to itself'; $_lang['ms3_err_no_link'] = 'Link type not found'; diff --git a/core/components/minishop3/lexicon/en/setting.inc.php b/core/components/minishop3/lexicon/en/setting.inc.php index a82d61935..892dbc979 100644 --- a/core/components/minishop3/lexicon/en/setting.inc.php +++ b/core/components/minishop3/lexicon/en/setting.inc.php @@ -139,6 +139,10 @@ $_lang['setting_ms3_status_paid_desc'] = 'What status to set after order payment'; $_lang['setting_ms3_status_canceled'] = 'Canceled order status ID'; $_lang['setting_ms3_status_canceled_desc'] = 'What status to set when canceling order'; +$_lang['setting_ms3_status_sent'] = 'Shipped / sent order status ID'; +$_lang['setting_ms3_status_sent_desc'] = 'Status ID treated as shipped for order lifecycle ports (default seed: 4).'; +$_lang['setting_ms3_order_status_transitions'] = 'Allowed order status transitions'; +$_lang['setting_ms3_order_status_transitions_desc'] = 'Optional allow-list of status edges in addition to final/fixed rules. Empty = no matrix (default final/fixed only). Format: CSV pairs from:to (e.g. 2:3,3:4,2:5) or JSON [[2,3],[3,4]].'; $_lang['setting_ms3_customer_cancel_allowed_statuses'] = 'Statuses from which customer can cancel order'; $_lang['setting_ms3_customer_cancel_allowed_statuses_desc'] = 'Comma-separated status IDs. Default: New and Paid (2,3). Empty = use ms3_status_new and ms3_status_paid.'; $_lang['setting_ms3_status_for_stat'] = 'Status IDs for statistics'; diff --git a/core/components/minishop3/lexicon/ru/default.inc.php b/core/components/minishop3/lexicon/ru/default.inc.php index 266ae656c..04be9deb1 100644 --- a/core/components/minishop3/lexicon/ru/default.inc.php +++ b/core/components/minishop3/lexicon/ru/default.inc.php @@ -193,6 +193,9 @@ $_lang['ms3_err_status_fixed'] = 'Установлен фиксирующий статус. Вы не можете сменить его на более ранний.'; $_lang['ms3_err_status_wrong'] = 'Неверный статус заказа.'; $_lang['ms3_err_status_same'] = 'Этот статус уже установлен.'; +$_lang['ms3_err_status_transition'] = 'Такой переход статуса не разрешён.'; +$_lang['ms3_err_status_transitions_invalid'] = 'Некорректный allow-list переходов статусов заказа.'; +$_lang['ms3_err_status_rollback'] = 'Не удалось откатить статус заказа после отклонённого перехода.'; $_lang['ms3_err_register_globals'] = 'Ошибка: php параметр register_globals должен быть выключен.'; $_lang['ms3_err_link_equal'] = 'Вы пытаетесь добавить товару ссылку на самого себя'; $_lang['ms3_err_no_link'] = 'Тип связи не найден'; diff --git a/core/components/minishop3/lexicon/ru/setting.inc.php b/core/components/minishop3/lexicon/ru/setting.inc.php index 88678f604..a1aea93f5 100644 --- a/core/components/minishop3/lexicon/ru/setting.inc.php +++ b/core/components/minishop3/lexicon/ru/setting.inc.php @@ -139,6 +139,10 @@ $_lang['setting_ms3_status_paid_desc'] = 'Какой статус нужно устанавливать после оплаты заказа'; $_lang['setting_ms3_status_canceled'] = 'ID статуса отмены заказа'; $_lang['setting_ms3_status_canceled_desc'] = 'Какой статус нужно устанавливать при отмене заказа'; +$_lang['setting_ms3_status_sent'] = 'ID статуса «Отправлен»'; +$_lang['setting_ms3_status_sent_desc'] = 'Статус, при котором срабатывает порт отгрузки в lifecycle (по умолчанию seed id 4).'; +$_lang['setting_ms3_order_status_transitions'] = 'Разрешённые переходы статусов заказа'; +$_lang['setting_ms3_order_status_transitions_desc'] = 'Опциональный allow-list рёбер поверх правил final/fixed. Пусто — только final/fixed. Формат: CSV пары from:to (например 2:3,3:4,2:5) или JSON [[2,3],[3,4]].'; $_lang['setting_ms3_customer_cancel_allowed_statuses'] = 'Статусы, из которых покупатель может отменить заказ'; $_lang['setting_ms3_customer_cancel_allowed_statuses_desc'] = 'ID статусов через запятую. По умолчанию: «Новый» и «Оплачен» (2,3). Пусто — использовать ms3_status_new и ms3_status_paid.'; $_lang['setting_ms3_status_for_stat'] = 'ID статусов для статистики'; diff --git a/core/components/minishop3/src/Controllers/Payment/Payment.php b/core/components/minishop3/src/Controllers/Payment/Payment.php index a0222b8ba..38f21d4c9 100644 --- a/core/components/minishop3/src/Controllers/Payment/Payment.php +++ b/core/components/minishop3/src/Controllers/Payment/Payment.php @@ -62,15 +62,32 @@ * } * * if ($data['status'] === 'succeeded') { - * $order->set('status_id', $this->getPaidStatusId()); - * $order->save(); - * return $this->success('Payment confirmed'); + * // Non-draft status changes must go through OrderStatusService (issue #592). + * // Prefer PaymentLifecycle (#590) when available; until then: + * $status = $this->modx->services->get('ms3_order_status'); + * $result = $status->change( + * (int) $order->get('id'), + * $this->getPaidStatusId(), + * false, + * ['idempotent' => true] + * ); + * return $result === true + * ? $this->success('Payment confirmed') + * : $this->error((string) $result); * } * * if ($data['status'] === 'canceled') { - * $order->set('status_id', $this->getCanceledStatusId()); - * $order->save(); - * return $this->error('Payment canceled'); + * $status = $this->modx->services->get('ms3_order_status'); + * $result = $status->change( + * (int) $order->get('id'), + * $this->getCanceledStatusId(), + * false, + * ['idempotent' => true] + * ); + * // ACK the webhook even when payment was canceled; distinguish transport vs business. + * return $result === true + * ? $this->success('Payment canceled') + * : $this->error((string) $result); * } * * return $this->error('Payment failed'); diff --git a/core/components/minishop3/src/ServiceRegistry.php b/core/components/minishop3/src/ServiceRegistry.php index 2e1e25643..a280c3afe 100644 --- a/core/components/minishop3/src/ServiceRegistry.php +++ b/core/components/minishop3/src/ServiceRegistry.php @@ -107,7 +107,7 @@ class ServiceRegistry 'ms3_order_number_generator', ], 'ms3_order_finalize' => ['ms3_order_number_generator'], - 'ms3_order_status' => ['ms3_order_log'], + 'ms3_order_status' => ['ms3_order_log', 'ms3_order_lifecycle_ports'], 'ms3_cart_mutation_handler' => [ 'ms3_order_draft_manager', 'ms3_cart_item_manager', @@ -260,6 +260,10 @@ class ServiceRegistry 'class' => \MiniShop3\Services\Order\OrderLogService::class, 'interface' => null, ], + 'ms3_order_lifecycle_ports' => [ + 'class' => \MiniShop3\Services\Order\NullOrderLifecyclePorts::class, + 'interface' => \MiniShop3\Services\Order\OrderLifecyclePortsInterface::class, + ], 'ms3_order_status' => [ 'class' => \MiniShop3\Services\Order\OrderStatusService::class, 'interface' => null, @@ -692,19 +696,16 @@ protected function validateClass( return $fallbackClass; } - if ($requiredInterface) { - $interfaces = class_implements($className); - if (!in_array($requiredInterface, $interfaces ?: [])) { - $this->modx->log( - modX::LOG_LEVEL_ERROR, - "[MiniShop3 ServiceRegistry] Class '{$className}' must implement {$requiredInterface}, " - . 'using fallback' - ); - return $fallbackClass; - } + if ($requiredInterface && !is_a($className, $requiredInterface, true)) { + $this->modx->log( + modX::LOG_LEVEL_ERROR, + "[MiniShop3 ServiceRegistry] Class '{$className}' must implement {$requiredInterface}, " + . 'using fallback' + ); + return $fallbackClass; } - if (!is_subclass_of($className, $fallbackClass)) { + if (!$requiredInterface && !is_subclass_of($className, $fallbackClass)) { $this->modx->log( modX::LOG_LEVEL_ERROR, "[MiniShop3 ServiceRegistry] Class '{$className}' must extend {$fallbackClass}, using fallback" diff --git a/core/components/minishop3/src/ServiceRegistryFactories.php b/core/components/minishop3/src/ServiceRegistryFactories.php index 522b9ac88..96b0ed925 100644 --- a/core/components/minishop3/src/ServiceRegistryFactories.php +++ b/core/components/minishop3/src/ServiceRegistryFactories.php @@ -148,11 +148,16 @@ public static function map(): array ); }, + 'ms3_order_lifecycle_ports' => static function (modX $modx, object $services, string $class): object { + return new $class(); + }, + 'ms3_order_status' => static function (modX $modx, object $services, string $class): object { return new $class( $modx, self::ms3($modx), - $services->get('ms3_order_log') + $services->get('ms3_order_log'), + $services->get('ms3_order_lifecycle_ports') ); }, diff --git a/core/components/minishop3/src/Services/Order/ManagerOrderMutationService.php b/core/components/minishop3/src/Services/Order/ManagerOrderMutationService.php index 769f417e3..f681cfaea 100644 --- a/core/components/minishop3/src/Services/Order/ManagerOrderMutationService.php +++ b/core/components/minishop3/src/Services/Order/ManagerOrderMutationService.php @@ -201,12 +201,19 @@ public function update(array $params = []): array // Store old values for logging $oldStatusId = (int)$order->get('status_id'); + $pendingStatusId = array_key_exists('status_id', $params) + ? (int) $params['status_id'] + : null; // Get editable order fields from msModelField configuration $orderFields = $this->presenter->getModelFieldNames('msOrder'); $changedOrderFields = []; foreach ($orderFields as $field) { + // Non-draft status changes go only through OrderStatusService (#592). + if ($field === 'status_id') { + continue; + } if (array_key_exists($field, $params)) { $oldValue = $order->get($field); $newValue = $params[$field]; @@ -257,8 +264,7 @@ public function update(array $params = []): array return $this->error('Failed to update order', HttpStatus::INTERNAL_SERVER_ERROR); } - // Log order field changes (excluding status_id which is logged separately) - unset($changedOrderFields['status_id']); + // Log order field changes (status_id is logged by OrderStatusService) if (!empty($changedOrderFields)) { $this->getOrderLog()->addEntry( $id, @@ -325,15 +331,10 @@ public function update(array $params = []): array } // Handle status change via OrderStatusService (sends notifications) - $newStatusId = (int)$order->get('status_id'); - if ($oldStatusId !== $newStatusId) { - // Revert status to old value - OrderStatusService will change it properly - $order->set('status_id', $oldStatusId); - $order->save(); - + if ($pendingStatusId !== null && $pendingStatusId !== $oldStatusId) { /** @var OrderStatusService $orderStatusService */ $orderStatusService = $this->modx->services->get('ms3_order_status'); - $result = $orderStatusService->change((int)$order->get('id'), $newStatusId); + $result = $orderStatusService->change((int)$order->get('id'), $pendingStatusId); if ($result !== true) { return $this->error( diff --git a/core/components/minishop3/src/Services/Order/NullOrderLifecyclePorts.php b/core/components/minishop3/src/Services/Order/NullOrderLifecyclePorts.php new file mode 100644 index 000000000..bc4df83d2 --- /dev/null +++ b/core/components/minishop3/src/Services/Order/NullOrderLifecyclePorts.php @@ -0,0 +1,28 @@ +modx = $modx; $this->ms3 = $ms3; $this->orderLog = $orderLog; + $this->lifecyclePorts = $lifecyclePorts ?? new NullOrderLifecyclePorts(); $this->modx->lexicon->load('minishop3:default'); } @@ -70,15 +84,22 @@ public function getAllowedCancelStatusIds(): array } /** - * Switch order status + * Switch order status (single gate for non-draft transitions). * * @param int $orderId The id of msOrder * @param int $statusId The id of msOrderStatus * @param bool $skipNotifications Skip sending notifications (for admin finalization) + * @param array{idempotent?: bool} $options idempotent=true → same status is success no-op * @return bool|string True on success, error message on failure */ - public function change(int $orderId, int $statusId, bool $skipNotifications = false): bool|string - { + public function change( + int $orderId, + int $statusId, + bool $skipNotifications = false, + array $options = [] + ): bool|string { + $idempotent = !empty($options['idempotent']); + /** @var msOrder|null $msOrder */ $msOrder = $this->modx->getObject(msOrder::class, ['id' => $orderId]); if (!$msOrder) { @@ -95,26 +116,26 @@ public function change(int $orderId, int $statusId, bool $skipNotifications = fa return $this->modx->lexicon('ms3_err_status_nf'); } + $storedStatusId = $msOrder->get('status_id'); + $previousStatusId = $storedStatusId !== null ? (int) $storedStatusId : null; + /** @var msOrderStatusModel|null $oldStatus */ - $oldStatus = $this->modx->getObject( - msOrderStatusModel::class, - ['id' => $msOrder->get('status_id'), 'active' => 1] - ); + $oldStatus = $previousStatusId !== null + ? $this->modx->getObject(msOrderStatusModel::class, ['id' => $previousStatusId]) + : null; - if ($oldStatus) { - $transitionError = $this->validateStatusTransition($oldStatus, $status); - if ($transitionError !== null) { - return $transitionError; - } + if ($previousStatusId === $statusId) { + return $idempotent ? true : $this->modx->lexicon('ms3_err_status_same'); } - if ($msOrder->get('status_id') == $statusId) { - return $this->modx->lexicon('ms3_err_status_same'); + $transitionError = $this->validateStatusTransition($oldStatus, $status); + if ($transitionError !== null) { + return $transitionError; } $eventParams = [ 'msOrder' => $msOrder, - 'old_status' => $oldStatus?->get('id'), + 'old_status' => $previousStatusId, 'status' => $statusId, ]; $response = $this->ms3->utils->invokeEvent('msOnBeforeChangeOrderStatus', $eventParams); @@ -134,8 +155,8 @@ public function change(int $orderId, int $statusId, bool $skipNotifications = fa if (!$status) { return $this->modx->lexicon('ms3_err_status_nf'); } - if ($msOrder->get('status_id') == $statusId) { - return $this->modx->lexicon('ms3_err_status_same'); + if ($previousStatusId === $statusId) { + return $idempotent ? true : $this->modx->lexicon('ms3_err_status_same'); } $transitionError = $this->validateStatusTransition($oldStatus, $status); @@ -145,22 +166,26 @@ public function change(int $orderId, int $statusId, bool $skipNotifications = fa } $msOrder->set('status_id', $statusId); - if (!$msOrder->save()) { return $this->modx->lexicon('ms3_err_unknown'); } - $this->orderLog->add($msOrder->get('id'), $statusId, 'status'); + $portError = $this->runLifecyclePorts($msOrder, $statusId, $previousStatusId); + if ($portError !== null) { + return $this->rollbackStatus($msOrder, $previousStatusId) ?? $portError; + } $response = $this->ms3->utils->invokeEvent('msOnChangeOrderStatus', [ 'msOrder' => $msOrder, - 'old_status' => $oldStatus?->get('id'), + 'old_status' => $previousStatusId, 'status' => $statusId, ]); if (!$response['success']) { - return $response['message']; + return $this->rollbackStatus($msOrder, $previousStatusId) ?? $response['message']; } + $this->orderLog->add($msOrder->get('id'), $statusId, 'status'); + // Send notifications via NotificationManager (unless skipped) // Use output buffering to prevent any stray output from Fenom/pdoTools if (!$skipNotifications) { @@ -173,7 +198,7 @@ public function change(int $orderId, int $statusId, bool $skipNotifications = fa } /** - * Validate transition from old status to new (final/fixed rules). + * Validate transition: final/fixed defaults + optional allow-list (ms3_order_status_transitions). */ protected function validateStatusTransition( ?msOrderStatusModel $oldStatus, @@ -191,9 +216,68 @@ protected function validateStatusTransition( return $this->modx->lexicon('ms3_err_status_fixed'); } + $edges = OrderStatusTransitionPolicy::resolve( + $this->modx->getOption('ms3_order_status_transitions', null, '') + ); + if ($edges['mode'] === OrderStatusTransitionPolicy::MODE_INVALID) { + return $this->modx->lexicon('ms3_err_status_transitions_invalid'); + } + if ( + $edges['mode'] === OrderStatusTransitionPolicy::MODE_ON + && !isset($edges['edges'][(int) $oldStatus->get('id')][(int) $newStatus->get('id')]) + ) { + return $this->modx->lexicon('ms3_err_status_transition'); + } + + return null; + } + + /** + * Invoke semantic lifecycle ports when the target matches configured status ids. + */ + protected function runLifecyclePorts(msOrder $order, int $statusId, ?int $previousStatusId): ?string + { + $paidId = (int) $this->modx->getOption('ms3_status_paid', null, 3); + $canceledId = (int) $this->modx->getOption('ms3_status_canceled', null, 5); + $sentId = (int) $this->modx->getOption('ms3_status_sent', null, 4); + + if ($statusId === $paidId) { + return $this->lifecyclePorts->onOrderBecamePaid($order, $previousStatusId); + } + if ($statusId === $canceledId) { + return $this->lifecyclePorts->onOrderCancelled($order, $previousStatusId); + } + if ($statusId === $sentId) { + return $this->lifecyclePorts->onOrderShipped($order, $previousStatusId); + } + return null; } + /** + * Restore previous status_id after failed after-event or lifecycle port. + * + * @return string|null Lexicon/error when rollback persist fails + */ + protected function rollbackStatus(msOrder $order, ?int $previousStatusId): ?string + { + $order->set('status_id', $previousStatusId); + if ($order->save()) { + return null; + } + + $this->modx->log( + modX::LOG_LEVEL_ERROR, + sprintf( + '[MiniShop3] Failed to rollback order #%s status to %s', + (string) $order->get('id'), + $previousStatusId === null ? 'null' : (string) $previousStatusId + ) + ); + + return $this->modx->lexicon('ms3_err_status_rollback'); + } + /** * Send notifications for status change * diff --git a/core/components/minishop3/src/Services/Order/OrderStatusTransitionPolicy.php b/core/components/minishop3/src/Services/Order/OrderStatusTransitionPolicy.php new file mode 100644 index 000000000..8d1eb13c0 --- /dev/null +++ b/core/components/minishop3/src/Services/Order/OrderStatusTransitionPolicy.php @@ -0,0 +1,89 @@ +>} + */ + public static function resolve(mixed $raw): array + { + if ($raw === null) { + return ['mode' => self::MODE_OFF, 'edges' => []]; + } + + if (is_array($raw)) { + return ['mode' => self::MODE_ON, 'edges' => self::fromPairList($raw)]; + } + + $value = trim((string) $raw); + if ($value === '') { + return ['mode' => self::MODE_OFF, 'edges' => []]; + } + + if (str_starts_with($value, '[')) { + $decoded = json_decode($value, true); + if (!is_array($decoded)) { + return ['mode' => self::MODE_INVALID, 'edges' => []]; + } + + return ['mode' => self::MODE_ON, 'edges' => self::fromPairList($decoded)]; + } + + $pairs = []; + foreach (array_filter(array_map('trim', explode(',', $value))) as $pair) { + $parts = array_map('trim', explode(':', $pair, 2)); + if (count($parts) !== 2) { + return ['mode' => self::MODE_INVALID, 'edges' => []]; + } + $pairs[] = $parts; + } + + $edges = self::fromPairList($pairs); + if ($edges === []) { + return ['mode' => self::MODE_INVALID, 'edges' => []]; + } + + return ['mode' => self::MODE_ON, 'edges' => $edges]; + } + + /** + * @param array $pairs + * @return array> + */ + private static function fromPairList(array $pairs): array + { + $edges = []; + foreach ($pairs as $pair) { + if (!is_array($pair) || count($pair) < 2) { + continue; + } + $from = (int) $pair[0]; + $to = (int) $pair[1]; + if ($from < 1 || $to < 1) { + continue; + } + $edges[$from][$to] = true; + } + + return $edges; + } +} diff --git a/core/components/minishop3/tests/Unit/Services/Order/OrderStatusServiceLifecycleTest.php b/core/components/minishop3/tests/Unit/Services/Order/OrderStatusServiceLifecycleTest.php new file mode 100644 index 000000000..67077110e --- /dev/null +++ b/core/components/minishop3/tests/Unit/Services/Order/OrderStatusServiceLifecycleTest.php @@ -0,0 +1,380 @@ +makeHarness(statusId: 2); + $log = $this->recordingLog(); + $events = []; + + $service = $this->makeService($harness, $log, new NullOrderLifecyclePorts(), $events); + $result = $service->change(10, 2, true, ['idempotent' => true]); + + self::assertTrue($result); + self::assertSame([], $log->entries); + self::assertSame([], $events); + } + + public function testSameStatusWithoutIdempotentReturnsError(): void + { + $harness = $this->makeHarness(statusId: 2); + $log = $this->recordingLog(); + $events = []; + + $service = $this->makeService($harness, $log, new NullOrderLifecyclePorts(), $events); + $result = $service->change(10, 2); + + self::assertSame('ms3_err_status_same', $result); + self::assertSame([], $log->entries); + } + + public function testAllowListRejectsUnknownEdgeWithoutSave(): void + { + $harness = $this->makeHarness(statusId: 2, options: [ + 'ms3_order_status_transitions' => '2:3', + ]); + $log = $this->recordingLog(); + $events = []; + + $service = $this->makeService($harness, $log, new NullOrderLifecyclePorts(), $events); + $result = $service->change(10, 4); + + self::assertSame('ms3_err_status_transition', $result); + self::assertSame(2, $harness['order']->get('status_id')); + self::assertSame([], $log->entries); + self::assertSame([], $events); + } + + public function testInvalidAllowListConfigRejectedWithoutSave(): void + { + $harness = $this->makeHarness(statusId: 2, options: [ + 'ms3_order_status_transitions' => '[broken', + ]); + $log = $this->recordingLog(); + $events = []; + + $service = $this->makeService($harness, $log, new NullOrderLifecyclePorts(), $events); + $result = $service->change(10, 3); + + self::assertSame('ms3_err_status_transitions_invalid', $result); + self::assertSame(2, $harness['order']->get('status_id')); + self::assertSame([], $log->entries); + } + + public function testCancelAndShippedPortsAreInvoked(): void + { + $ports = new class implements OrderLifecyclePortsInterface { + public int $cancelCalls = 0; + public int $shipCalls = 0; + + public function onOrderBecamePaid(msOrder $order, ?int $previousStatusId): ?string + { + return null; + } + + public function onOrderCancelled(msOrder $order, ?int $previousStatusId): ?string + { + ++$this->cancelCalls; + + return null; + } + + public function onOrderShipped(msOrder $order, ?int $previousStatusId): ?string + { + ++$this->shipCalls; + + return null; + } + }; + + $cancelHarness = $this->makeHarness(statusId: 2); + $cancelLog = $this->recordingLog(); + $cancelEvents = []; + $cancelService = $this->makeService($cancelHarness, $cancelLog, $ports, $cancelEvents); + self::assertTrue($cancelService->change(10, 5, true)); + self::assertSame(1, $ports->cancelCalls); + + $shipHarness = $this->makeHarness(statusId: 2); + $shipLog = $this->recordingLog(); + $shipEvents = []; + $shipService = $this->makeService($shipHarness, $shipLog, $ports, $shipEvents); + self::assertTrue($shipService->change(10, 4, true)); + self::assertSame(1, $ports->shipCalls); + } + + public function testAfterEventFailureRollsBackStatusAndSkipsLog(): void + { + $harness = $this->makeHarness(statusId: 2); + $log = $this->recordingLog(); + $events = []; + + $service = $this->makeService( + $harness, + $log, + new NullOrderLifecyclePorts(), + $events, + afterFail: true + ); + $result = $service->change(10, 3, true); + + self::assertSame('after failed', $result); + self::assertSame(2, $harness['order']->get('status_id')); + self::assertSame([], $log->entries); + self::assertSame(['msOnBeforeChangeOrderStatus', 'msOnChangeOrderStatus'], $events); + } + + public function testPaidPortRunsAndFailureRollsBack(): void + { + $harness = $this->makeHarness(statusId: 2); + $log = $this->recordingLog(); + $events = []; + $ports = new class implements OrderLifecyclePortsInterface { + public int $paidCalls = 0; + + public function onOrderBecamePaid(msOrder $order, ?int $previousStatusId): ?string + { + ++$this->paidCalls; + + return 'port failed'; + } + + public function onOrderCancelled(msOrder $order, ?int $previousStatusId): ?string + { + return null; + } + + public function onOrderShipped(msOrder $order, ?int $previousStatusId): ?string + { + return null; + } + }; + + $service = $this->makeService($harness, $log, $ports, $events); + $result = $service->change(10, 3, true); + + self::assertSame('port failed', $result); + self::assertSame(1, $ports->paidCalls); + self::assertSame(2, $harness['order']->get('status_id')); + self::assertSame([], $log->entries); + self::assertSame(['msOnBeforeChangeOrderStatus'], $events); + } + + public function testSuccessfulPaidTransitionLogsAndInvokesPort(): void + { + $harness = $this->makeHarness(statusId: 2); + $log = $this->recordingLog(); + $events = []; + $ports = new class implements OrderLifecyclePortsInterface { + public int $paidCalls = 0; + + public function onOrderBecamePaid(msOrder $order, ?int $previousStatusId): ?string + { + ++$this->paidCalls; + + return null; + } + + public function onOrderCancelled(msOrder $order, ?int $previousStatusId): ?string + { + return null; + } + + public function onOrderShipped(msOrder $order, ?int $previousStatusId): ?string + { + return null; + } + }; + + $service = $this->makeService($harness, $log, $ports, $events); + $result = $service->change(10, 3, true); + + self::assertTrue($result); + self::assertSame(1, $ports->paidCalls); + self::assertSame(3, $harness['order']->get('status_id')); + self::assertSame([[10, 3, 'status']], $log->entries); + self::assertSame(['msOnBeforeChangeOrderStatus', 'msOnChangeOrderStatus'], $events); + } + + /** + * @param array $options + * @return array{order: RecordingMsOrder, statuses: array, options: array} + */ + private function makeHarness(int $statusId, array $options = []): array + { + $order = new RecordingMsOrder([ + 'id' => 10, + 'status_id' => $statusId, + 'context' => 'web', + ]); + + $statuses = [ + 2 => $this->makeStatus(2, final: false, fixed: false, position: 2), + 3 => $this->makeStatus(3, final: false, fixed: true, position: 3), + 4 => $this->makeStatus(4, final: true, fixed: true, position: 4), + 5 => $this->makeStatus(5, final: true, fixed: false, position: 5), + ]; + + return [ + 'order' => $order, + 'statuses' => $statuses, + 'options' => array_merge([ + 'ms3_status_paid' => 3, + 'ms3_status_canceled' => 5, + 'ms3_status_sent' => 4, + 'ms3_order_status_transitions' => '', + ], $options), + ]; + } + + private function makeStatus(int $id, bool $final, bool $fixed, int $position): object + { + return new class($id, $final, $fixed, $position) extends msOrderStatus { + public function __construct( + private int $statusId, + private bool $isFinal, + private bool $isFixed, + private int $pos + ) { + } + + public function get($k, $format = null, $formatType = '') + { + return match ($k) { + 'id' => $this->statusId, + 'final' => $this->isFinal ? 1 : 0, + 'fixed' => $this->isFixed ? 1 : 0, + 'position' => $this->pos, + default => null, + }; + } + }; + } + + /** + * @return OrderLogService&object{entries: list} + */ + private function recordingLog(): OrderLogService + { + return new class extends OrderLogService { + /** @var list */ + public array $entries = []; + + public function __construct() + { + } + + public function add(int $order_id, mixed $entry, string $action, bool $visible = true): bool + { + $this->entries[] = [$order_id, $entry, $action]; + + return true; + } + }; + } + + /** + * @param array{order: RecordingMsOrder, statuses: array, options: array} $harness + * @param list $events + */ + private function makeService( + array $harness, + OrderLogService $log, + OrderLifecyclePortsInterface $ports, + array &$events, + bool $afterFail = false + ): OrderStatusService { + $modx = new class($harness, $afterFail) extends modX { + /** @var array{order: RecordingMsOrder, statuses: array, options: array} */ + private array $harness; + private bool $afterFail; + + public function __construct(array $harness, bool $afterFail) + { + parent::__construct(); + $this->harness = $harness; + $this->afterFail = $afterFail; + } + + public function getOption(string $key, $options = null, $default = null) + { + return $this->harness['options'][$key] ?? $default; + } + + public function switchContext($contextKey, $force = false) + { + return true; + } + + public function getObject($className, $criteria = null, $cacheFlag = true) + { + if ($className === msOrder::class || $className === RecordingMsOrder::class) { + $id = is_array($criteria) ? (int) ($criteria['id'] ?? 0) : (int) $criteria; + return $id === (int) $this->harness['order']->get('id') + ? $this->harness['order'] + : null; + } + + if ($className === msOrderStatus::class || is_a($className, msOrderStatus::class, true)) { + $id = is_array($criteria) ? (int) ($criteria['id'] ?? 0) : (int) $criteria; + return $this->harness['statuses'][$id] ?? null; + } + + return null; + } + }; + + $ms3 = $this->createMock(MiniShop3::class); + $ms3->method('initialize')->willReturn(true); + + $utils = new class($events, $afterFail) { + /** @var list */ + private array $events; + private bool $afterFail; + + public function __construct(array &$events, bool $afterFail) + { + $this->events = &$events; + $this->afterFail = $afterFail; + } + + public function invokeEvent(string $eventName, array $params = [], $glue = '
'): array + { + $this->events[] = $eventName; + if ($eventName === 'msOnChangeOrderStatus' && $this->afterFail) { + return ['success' => false, 'message' => 'after failed', 'data' => []]; + } + + return ['success' => true, 'message' => '', 'data' => $params]; + } + }; + $ms3->utils = $utils; + + return new OrderStatusService($modx, $ms3, $log, $ports); + } +} diff --git a/core/components/minishop3/tests/Unit/Services/Order/OrderStatusTransitionPolicyTest.php b/core/components/minishop3/tests/Unit/Services/Order/OrderStatusTransitionPolicyTest.php new file mode 100644 index 000000000..4f511ee3b --- /dev/null +++ b/core/components/minishop3/tests/Unit/Services/Order/OrderStatusTransitionPolicyTest.php @@ -0,0 +1,57 @@ + Date: Sun, 6 Sep 2026 08:27:29 +0200 Subject: [PATCH 03/19] =?UTF-8?q?PR=20#598:=20feat(web-api):=20=D0=B3?= =?UTF-8?q?=D0=B0=D0=BB=D0=B5=D1=80=D0=B5=D1=8F=20=D0=B8=D0=B7=D0=BE=D0=B1?= =?UTF-8?q?=D1=80=D0=B0=D0=B6=D0=B5=D0=BD=D0=B8=D0=B9=20=D1=82=D0=BE=D0=B2?= =?UTF-8?q?=D0=B0=D1=80=D0=B0=20(images[])?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit https://github.com/modx-pro/MiniShop3/pull/598 --- .../minishop3/config/routes/web.php | 5 + .../Controllers/Api/Web/ProductController.php | 35 +++- .../src/Middleware/TokenMiddleware.php | 39 ++++ .../minishop3/src/ServiceRegistry.php | 4 + .../src/ServiceRegistryFactories.php | 1 + .../Product/ProductCatalogService.php | 125 +++++++++++- .../ProductGalleryPublicSerializer.php | 158 +++++++++++++++ .../Product/ProductGalleryPublicService.php | 186 ++++++++++++++++++ .../tests/ProductCatalogImagesRoutesTest.php | 91 +++++++++ .../tests/ProductCatalogServiceTest.php | 22 ++- .../tests/TokenMiddlewarePublicRoutesTest.php | 8 + .../TokenMiddlewarePublicPatternTest.php | 57 ++++++ .../ProductGalleryPublicSerializerTest.php | 120 +++++++++++ 13 files changed, 840 insertions(+), 11 deletions(-) create mode 100644 core/components/minishop3/src/Services/Product/ProductGalleryPublicSerializer.php create mode 100644 core/components/minishop3/src/Services/Product/ProductGalleryPublicService.php create mode 100644 core/components/minishop3/tests/ProductCatalogImagesRoutesTest.php create mode 100644 core/components/minishop3/tests/Unit/Middleware/TokenMiddlewarePublicPatternTest.php create mode 100644 core/components/minishop3/tests/Unit/Services/Product/ProductGalleryPublicSerializerTest.php diff --git a/core/components/minishop3/config/routes/web.php b/core/components/minishop3/config/routes/web.php index 39dfd9ed1..a2e998765 100644 --- a/core/components/minishop3/config/routes/web.php +++ b/core/components/minishop3/config/routes/web.php @@ -284,6 +284,11 @@ $controller = new \MiniShop3\Controllers\Api\Web\ProductController($modx); return $controller->filters($params); }); + + $router->get('/{id}/images', function ($params) use ($modx) { + $controller = new \MiniShop3\Controllers\Api\Web\ProductController($modx); + return $controller->getImages($params); + }); }); // Public category catalog — no TokenMiddleware (headless nav / PLP) diff --git a/core/components/minishop3/src/Controllers/Api/Web/ProductController.php b/core/components/minishop3/src/Controllers/Api/Web/ProductController.php index 35a2164c0..a9c308312 100644 --- a/core/components/minishop3/src/Controllers/Api/Web/ProductController.php +++ b/core/components/minishop3/src/Controllers/Api/Web/ProductController.php @@ -29,6 +29,8 @@ public function __construct(modX $modx) /** * GET /api/v1/product/get/{id} * + * Query: context, include_images (0|1, default 0 — omit images[]; name→alt, no DB alt). + * * @param array $params */ public function get(array $params = []): Response @@ -59,7 +61,8 @@ public function get(array $params = []): Response * * Query: parent|category, parents, nested, price_min, price_max, in_stock, stock_min, * vendor_id, new, popular, favorite, options (JSON), - * limit, offset|page, sort, dir, query, context, include_options, include_content + * limit, offset|page, sort, dir, query, context, include_options, include_content, + * include_images (0|1, default 0, cap 10 files per item) * * @param array $params Route + query params (Router merges $_GET) */ @@ -99,6 +102,36 @@ public function filters(array $params = []): Response return Response::success($result); } + /** + * GET /api/v1/product/{id}/images + * + * Same gallery serializer as include_images=1 on get. 404 if the product is not storefront-visible. + * + * @param array $params + */ + public function getImages(array $params = []): Response + { + $productId = (int) ($params['id'] ?? 0); + + if ($productId <= 0) { + return Response::error( + $this->modx->lexicon('ms3_err_product_id_ns'), + HttpStatus::BAD_REQUEST + ); + } + + $result = $this->catalog()->getPublicImages($productId, $params); + + if ($result === null) { + return Response::error( + $this->modx->lexicon('ms3_err_product_nf'), + HttpStatus::NOT_FOUND + ); + } + + return Response::success($result); + } + private function catalog(): ProductCatalogService { /** @var ProductCatalogService $service */ diff --git a/core/components/minishop3/src/Middleware/TokenMiddleware.php b/core/components/minishop3/src/Middleware/TokenMiddleware.php index 5c035a923..6550d8a07 100644 --- a/core/components/minishop3/src/Middleware/TokenMiddleware.php +++ b/core/components/minishop3/src/Middleware/TokenMiddleware.php @@ -48,6 +48,16 @@ class TokenMiddleware implements MiddlewareInterface '/api/v1/health', ]; + /** + * Glob patterns (`*` = one path segment). Keeps /{id}/images public without + * opening the whole `/api/v1/product/*` group via a prefix (#584). + * + * @var list + */ + private array $publicRoutePatterns = [ + '/api/v1/product/*/images', + ]; + /** * @param modX $modx MODX instance */ @@ -196,15 +206,44 @@ private function isPublicRoute(string $uri): bool $route = preg_replace('#^/assets/components/minishop3/api\.php#', '', $path); } + $route = $this->normalizePublicPath((string) $route); + foreach ($this->publicRoutes as $publicRoute) { if (str_starts_with($route, $publicRoute)) { return true; } } + foreach ($this->publicRoutePatterns as $pattern) { + if (self::matchesSegmentPattern($route, $pattern)) { + return true; + } + } + return false; } + private function normalizePublicPath(string $route): string + { + $qPos = strpos($route, '?'); + if ($qPos !== false) { + $route = substr($route, 0, $qPos); + } + + if ($route !== '/') { + $route = rtrim($route, '/'); + } + + return $route; + } + + private static function matchesSegmentPattern(string $path, string $pattern): bool + { + $regex = '#^' . str_replace('\\*', '[^/]+', preg_quote($pattern, '#')) . '$#'; + + return (bool) preg_match($regex, $path); + } + /** * Add public route * diff --git a/core/components/minishop3/src/ServiceRegistry.php b/core/components/minishop3/src/ServiceRegistry.php index a280c3afe..c65eb80fe 100644 --- a/core/components/minishop3/src/ServiceRegistry.php +++ b/core/components/minishop3/src/ServiceRegistry.php @@ -162,6 +162,10 @@ class ServiceRegistry 'class' => \MiniShop3\Services\Product\ProductFacetService::class, 'interface' => null, ], + 'ms3_product_gallery_public' => [ + 'class' => \MiniShop3\Services\Product\ProductGalleryPublicService::class, + 'interface' => null, + ], 'ms3_category_catalog' => [ 'class' => \MiniShop3\Services\Category\CategoryCatalogService::class, 'interface' => null, diff --git a/core/components/minishop3/src/ServiceRegistryFactories.php b/core/components/minishop3/src/ServiceRegistryFactories.php index 96b0ed925..074b51749 100644 --- a/core/components/minishop3/src/ServiceRegistryFactories.php +++ b/core/components/minishop3/src/ServiceRegistryFactories.php @@ -40,6 +40,7 @@ public static function map(): array 'ms3_product_link_service' => $modxOnly(), 'ms3_product_catalog' => $modxOnly(), 'ms3_product_facets' => $modxOnly(), + 'ms3_product_gallery_public' => $modxOnly(), 'ms3_category_catalog' => $modxOnly(), 'ms3_delivery_catalog' => $modxOnly(), 'ms3_payment_catalog' => $modxOnly(), diff --git a/core/components/minishop3/src/Services/Product/ProductCatalogService.php b/core/components/minishop3/src/Services/Product/ProductCatalogService.php index 2eb6e11b8..652ba434c 100644 --- a/core/components/minishop3/src/Services/Product/ProductCatalogService.php +++ b/core/components/minishop3/src/Services/Product/ProductCatalogService.php @@ -130,6 +130,7 @@ public static function whitelistPublicPayload( array $payload, bool $includeContent, bool $includeOptions, + bool $includeImages = false, ): array { $allowed = array_merge(self::RESOURCE_FIELDS, self::DATA_FIELDS); if ($includeContent) { @@ -147,6 +148,10 @@ public static function whitelistPublicPayload( $result['options'] = self::stripOptionMetadata($payload['options']); } + if ($includeImages && is_array($payload['images'] ?? null)) { + $result['images'] = ProductGalleryPublicSerializer::whitelistItems($payload['images']); + } + return $result; } @@ -158,10 +163,48 @@ public static function toBool(mixed $value): bool /** * Single published product by ID (same visibility rules as list). * + * Query: context, include_images (0|1, default 0). + * * @param array $params Optional context override * @return array|null */ public function getById(int $productId, array $params = []): ?array + { + $product = $this->findVisibleProduct($productId, $params); + if ($product === null) { + return null; + } + + $options = self::stripOptionMetadata( + $this->optionService()->loadOptionsForProduct($productId, false) + ); + + $includeImages = self::toBool($params['include_images'] ?? false); + $images = $includeImages ? $this->loadImagesForProduct($product) : null; + + return $this->formatProduct($product, true, $options, $images); + } + + /** + * Gallery only: same visibility as get. Always returns images[] (may be empty). + * + * @param array $params + * @return array{images: list>}|null + */ + public function getPublicImages(int $productId, array $params = []): ?array + { + $product = $this->findVisibleProduct($productId, $params); + if ($product === null) { + return null; + } + + return ['images' => $this->loadImagesForProduct($product)]; + } + + /** + * @param array $params + */ + private function findVisibleProduct(int $productId, array $params): ?msProduct { if ($productId <= 0) { return null; @@ -176,15 +219,46 @@ public function getById(int $productId, array $params = []): ?array /** @var msProduct|null $product */ $product = $this->modx->getObject(msProduct::class, $this->publicCriteria($criteria)); - if (!$product) { - return null; - } + return $product ?: null; + } - $options = self::stripOptionMetadata( - $this->optionService()->loadOptionsForProduct($productId, false) + /** + * @return list> + */ + private function loadImagesForProduct(msProduct $product): array + { + $data = $product->loadData(); + $previewFileId = $data + ? $this->imageService()->resolvePreviewFileId($data) + : 0; + + return $this->gallery()->loadForProduct( + (int) $product->get('id'), + (string) $product->get('pagetitle'), + $previewFileId, ); + } + + /** + * @param list $products + * @return array>> + */ + private function loadImagesForProducts(array $products): array + { + $meta = []; + foreach ($products as $product) { + $id = (int) $product->get('id'); + if ($id <= 0) { + continue; + } + $data = $product->loadData(); + $meta[$id] = [ + 'pagetitle' => (string) $product->get('pagetitle'), + 'preview_file_id' => $data ? (int) $data->get('preview_file_id') : 0, + ]; + } - return $this->formatProduct($product, true, $options); + return $this->gallery()->loadForProducts($meta, ProductGalleryPublicSerializer::MAX_IMAGES_LIST); } /** @@ -198,7 +272,7 @@ public function getById(int $productId, array $params = []): ?array * - in_stock, stock_min, vendor_id, new, popular, favorite * - options: JSON object or bracket map (AND between keys, OR within key) * - limit, offset | page, sort, dir, query, context - * - include_options, include_content + * - include_options, include_content, include_images (default 0; cap 10 files / product) * * @param array $params * @return array{items: list>, total: int, limit: int, offset: int} @@ -213,6 +287,7 @@ public function getList(array $params): array $offset = self::resolveOffset($params, $limit); $includeOptions = self::toBool($params['include_options'] ?? false); $includeContent = self::toBool($params['include_content'] ?? false); + $includeImages = self::toBool($params['include_images'] ?? false); $total = $this->countList($params, $filters); @@ -230,11 +305,16 @@ public function getList(array $params): array ? $this->loadOptionsForProducts($ids) : []; + $galleries = ($includeImages && $ids !== []) + ? $this->loadImagesForProducts($productList) + : []; + $items = []; foreach ($productList as $product) { $productId = (int) $product->get('id'); $options = $includeOptions ? ($optionsByProduct[$productId] ?? []) : null; - $items[] = $this->formatProduct($product, $includeContent, $options); + $images = $includeImages ? ($galleries[$productId] ?? []) : null; + $items[] = $this->formatProduct($product, $includeContent, $options, $images); } return [ @@ -439,12 +519,14 @@ private function optionService(): OptionService /** * @param array|null $options null = omit options key; array = include + * @param list>|null $images null = omit images key; array = include * @return array */ private function formatProduct( msProduct $product, bool $includeContent, ?array $options = null, + ?array $images = null, ): array { $data = $product->loadData(); $payload = []; @@ -482,11 +564,36 @@ private function formatProduct( $payload['options'] = $options; } + if ($images !== null) { + $payload['images'] = $images; + } + $modified = $product->modifyFields($payload); if (is_array($modified)) { $payload = $modified; } - return self::whitelistPublicPayload($payload, $includeContent, $options !== null); + return self::whitelistPublicPayload( + $payload, + $includeContent, + $options !== null, + $images !== null, + ); + } + + private function gallery(): ProductGalleryPublicService + { + /** @var ProductGalleryPublicService $service */ + $service = $this->modx->services->get('ms3_product_gallery_public'); + + return $service; + } + + private function imageService(): ProductImageService + { + /** @var ProductImageService $service */ + $service = $this->modx->services->get('ms3_product_image'); + + return $service; } } diff --git a/core/components/minishop3/src/Services/Product/ProductGalleryPublicSerializer.php b/core/components/minishop3/src/Services/Product/ProductGalleryPublicSerializer.php new file mode 100644 index 000000000..ced7b702a --- /dev/null +++ b/core/components/minishop3/src/Services/Product/ProductGalleryPublicSerializer.php @@ -0,0 +1,158 @@ + */ + public const ITEM_KEYS = [ + 'id', + 'url', + 'thumb', + 'thumbs', + 'name', + 'description', + 'alt', + 'position', + 'is_preview', + ]; + + /** + * Size folder from child path (`{productId}/{size}/`) or url (`/{id}/{size}/`). + */ + public static function sizeKeyFromChild(string $path, string $url, int $productId): string + { + $normalized = trim(str_replace('\\', '/', $path), '/'); + if ($normalized !== '') { + $parts = explode('/', $normalized); + $last = (string) end($parts); + if ($last !== '' && ($productId <= 0 || $last !== (string) $productId)) { + return $last; + } + } + + if ($productId > 0 && preg_match('#/' . preg_quote((string) $productId, '#') . '/([^/]+)/#', $url, $m) === 1) { + return $m[1]; + } + + return ''; + } + + /** + * @param list> $originals Top-level files (already filtered/sorted/capped) + * @param array}> $thumbsByParent + * @return list> + */ + public static function serializeGallery( + array $originals, + array $thumbsByParent, + string $pagetitle, + int $previewFileId, + ): array { + $validIds = []; + foreach ($originals as $row) { + $id = (int) ($row['id'] ?? 0); + if ($id > 0) { + $validIds[] = $id; + } + } + $effectivePreview = $previewFileId; + if ($effectivePreview <= 0 || !in_array($effectivePreview, $validIds, true)) { + $effectivePreview = $validIds[0] ?? 0; + } + + $items = []; + foreach ($originals as $row) { + $id = (int) ($row['id'] ?? 0); + if ($id <= 0) { + continue; + } + + $url = (string) ($row['url'] ?? ''); + $name = trim((string) ($row['name'] ?? '')); + $bundle = $thumbsByParent[$id] ?? []; + $thumb = trim((string) ($bundle['thumb'] ?? '')); + $thumbs = self::whitelistThumbs($bundle['thumbs'] ?? []); + + $items[] = [ + 'id' => $id, + 'url' => $url, + 'thumb' => $thumb !== '' ? $thumb : $url, + 'thumbs' => $thumbs, + 'name' => $name, + 'description' => (string) ($row['description'] ?? ''), + 'alt' => $name !== '' ? $name : $pagetitle, + 'position' => (int) ($row['position'] ?? 0), + 'is_preview' => $effectivePreview > 0 && $id === $effectivePreview, + ]; + } + + return $items; + } + + /** + * Drop leaked file internals if a plugin mutates images[]. + * + * @param list $images + * @return list> + */ + public static function whitelistItems(array $images): array + { + $out = []; + foreach ($images as $item) { + if (!is_array($item)) { + continue; + } + + $clean = []; + foreach (self::ITEM_KEYS as $key) { + if (!array_key_exists($key, $item)) { + continue; + } + if ($key === 'thumbs') { + $clean['thumbs'] = self::whitelistThumbs($item['thumbs']); + continue; + } + $clean[$key] = $item[$key]; + } + + if ($clean !== []) { + $out[] = $clean; + } + } + + return $out; + } + + /** + * @return array + */ + public static function whitelistThumbs(mixed $thumbs): array + { + if (!is_array($thumbs)) { + return []; + } + + $out = []; + foreach ($thumbs as $size => $url) { + if ($size === '' || (!is_scalar($url) && $url !== null)) { + continue; + } + $out[(string) $size] = (string) $url; + } + + return $out; + } +} diff --git a/core/components/minishop3/src/Services/Product/ProductGalleryPublicService.php b/core/components/minishop3/src/Services/Product/ProductGalleryPublicService.php new file mode 100644 index 000000000..fcf80c14e --- /dev/null +++ b/core/components/minishop3/src/Services/Product/ProductGalleryPublicService.php @@ -0,0 +1,186 @@ +> + */ + public function loadForProduct(int $productId, string $pagetitle, int $previewFileId): array + { + return $this->loadForProducts( + [$productId => ['pagetitle' => $pagetitle, 'preview_file_id' => $previewFileId]], + ProductGalleryPublicSerializer::MAX_IMAGES, + )[$productId] ?? []; + } + + /** + * @param array $products + * @return array>> + */ + public function loadForProducts(array $products, int $perProductLimit): array + { + $ids = []; + foreach (array_keys($products) as $id) { + $id = (int) $id; + if ($id > 0) { + $ids[] = $id; + } + } + if ($ids === []) { + return []; + } + + $limit = max(1, $perProductLimit); + $grouped = $this->groupOriginals($this->fetchOriginalsForProducts($ids), $limit); + $parentIds = []; + foreach ($grouped as $rows) { + foreach ($rows as $row) { + $parentIds[] = (int) ($row['id'] ?? 0); + } + } + $thumbs = $this->fetchThumbsByParent(array_values(array_filter($parentIds))); + + $out = []; + foreach ($products as $id => $meta) { + $id = (int) $id; + if ($id <= 0) { + continue; + } + $out[$id] = ProductGalleryPublicSerializer::serializeGallery( + $grouped[$id] ?? [], + $thumbs, + (string) ($meta['pagetitle'] ?? ''), + (int) ($meta['preview_file_id'] ?? 0), + ); + } + + return $out; + } + + /** + * @param list $productIds + * @return list> + */ + private function fetchOriginalsForProducts(array $productIds): array + { + $c = $this->modx->newQuery(msProductFile::class); + $c->where([ + 'product_id:IN' => $productIds, + 'parent_id' => 0, + 'type' => 'image', + 'active' => 1, + ]); + $c->select('id, product_id, url, name, description, position'); + $c->sortby('product_id', 'ASC'); + $c->sortby('position', 'ASC'); + $c->sortby('id', 'ASC'); + + if (!$c->prepare() || !$c->stmt->execute()) { + return []; + } + + return $c->stmt->fetchAll(\PDO::FETCH_ASSOC) ?: []; + } + + /** + * @param list> $rows + * @return array>> + */ + private function groupOriginals(array $rows, int $perProductLimit): array + { + $grouped = []; + foreach ($rows as $row) { + $productId = (int) ($row['product_id'] ?? 0); + if ($productId <= 0) { + continue; + } + if (!isset($grouped[$productId])) { + $grouped[$productId] = []; + } + if (count($grouped[$productId]) >= $perProductLimit) { + continue; + } + $grouped[$productId][] = $row; + } + + return $grouped; + } + + /** + * @param list $parentIds + * @return array}> + */ + private function fetchThumbsByParent(array $parentIds): array + { + if ($parentIds === []) { + return []; + } + + $c = $this->modx->newQuery(msProductFile::class); + $c->where([ + 'parent_id:IN' => $parentIds, + 'type' => 'image', + 'active' => 1, + ]); + $c->select('id, parent_id, product_id, url, path'); + $c->sortby('id', 'ASC'); + + if (!$c->prepare() || !$c->stmt->execute()) { + return []; + } + + $preferred = $this->preferredThumbSize(); + $out = []; + foreach ($c->stmt->fetchAll(\PDO::FETCH_ASSOC) ?: [] as $row) { + $parentId = (int) ($row['parent_id'] ?? 0); + if ($parentId <= 0) { + continue; + } + $url = (string) ($row['url'] ?? ''); + $size = ProductGalleryPublicSerializer::sizeKeyFromChild( + (string) ($row['path'] ?? ''), + $url, + (int) ($row['product_id'] ?? 0), + ); + if (!isset($out[$parentId])) { + $out[$parentId] = ['thumb' => '', 'thumbs' => []]; + } + if ($size !== '' && !isset($out[$parentId]['thumbs'][$size])) { + $out[$parentId]['thumbs'][$size] = $url; + } + if ($size === $preferred) { + $out[$parentId]['thumb'] = $url; + } elseif ($out[$parentId]['thumb'] === '') { + $out[$parentId]['thumb'] = $url; + } + } + + return $out; + } + + private function preferredThumbSize(): string + { + $size = trim((string) $this->modx->getOption('ms3_product_thumbnail_size', null, 'small')); + + return $size !== '' ? $size : 'small'; + } +} diff --git a/core/components/minishop3/tests/ProductCatalogImagesRoutesTest.php b/core/components/minishop3/tests/ProductCatalogImagesRoutesTest.php new file mode 100644 index 000000000..68f837a22 --- /dev/null +++ b/core/components/minishop3/tests/ProductCatalogImagesRoutesTest.php @@ -0,0 +1,91 @@ + $catalog, + 'controller' => $controller, + 'service' => $service, + 'serializer' => $serializer, + 'webRoutes' => $webRoutes, + ] as $label => $src +) { + if ($src === false || $src === '') { + $fail("unable to read {$label}"); + } +} + +if (!str_contains($catalog, 'include_images')) { + $fail('ProductCatalogService must honor include_images'); +} +if (!str_contains($catalog, 'ProductGalleryPublicSerializer::whitelistItems')) { + $fail('images must pass serializer allowlist after plugins'); +} +if (!str_contains($catalog, 'resolvePreviewFileId')) { + $fail('is_preview must use ProductImageService::resolvePreviewFileId'); +} +if (!str_contains($catalog, "ms3_product_gallery_public")) { + $fail('gallery service must come from ServiceRegistry'); +} +if (!preg_match('/function getById\([\s\S]*?loadImagesForProduct/', $catalog)) { + $fail('gallery load must run inside getById after product is found'); +} +if (!preg_match('/function getList\([\s\S]*?loadImagesForProducts/', $catalog)) { + $fail('product/list must batch-load gallery when include_images is on'); +} +if (!str_contains($catalog, 'function getPublicImages')) { + $fail('catalog must expose getPublicImages for /product/{id}/images'); +} + +if (!str_contains($controller, 'function getImages')) { + $fail('ProductController must expose getImages'); +} +if (!str_contains($webRoutes, "'/{id}/images'") && !str_contains($webRoutes, '"/{id}/images"')) { + $fail('web.php must register GET /product/{id}/images'); +} + +$registry = file_get_contents(__DIR__ . '/../src/ServiceRegistry.php'); +$factories = file_get_contents(__DIR__ . '/../src/ServiceRegistryFactories.php'); +if ($registry === false || $factories === false + || !str_contains($registry, "'ms3_product_gallery_public'") + || !str_contains($factories, "'ms3_product_gallery_public'")) { + $fail('ms3_product_gallery_public must be registered'); +} + +if (!str_contains($service, "'parent_id' => 0") || !str_contains($service, "'active' => 1")) { + $fail('gallery query must restrict parent_id=0 and active=1'); +} +if (!str_contains($service, 'parent_id:IN')) { + $fail('thumbs must batch-load via parent_id IN'); +} +if (!str_contains($service, 'ms3_product_thumbnail_size')) { + $fail('thumb must prefer ms3_product_thumbnail_size'); +} + +if (!str_contains($serializer, 'no DB `alt`')) { + $fail('serializer must document name → alt mapping'); +} +if (!str_contains($serializer, "'thumbs'")) { + $fail('serializer must expose multi-size thumbs map'); +} + +fwrite(STDOUT, "OK ProductCatalogImagesRoutesTest\n"); +exit(0); diff --git a/core/components/minishop3/tests/ProductCatalogServiceTest.php b/core/components/minishop3/tests/ProductCatalogServiceTest.php index 30d36c0b3..90e36054e 100644 --- a/core/components/minishop3/tests/ProductCatalogServiceTest.php +++ b/core/components/minishop3/tests/ProductCatalogServiceTest.php @@ -100,9 +100,29 @@ 'pagetitle' => 'Coffee', 'content' => '

hidden

', 'options' => ['size' => ['L']], + 'images' => [['id' => 1, 'hash' => 'x']], 'tv_private' => 'x', ], false, false), - 'whitelist omits content/options when flags off' + 'whitelist omits content/options/images when flags off' +); + +$assertSame( + [ + 'id' => 3, + 'pagetitle' => 'Mug', + 'images' => [ + ['id' => 9, 'url' => '/m.jpg'], + ], + ], + ProductCatalogService::whitelistPublicPayload([ + 'id' => 3, + 'pagetitle' => 'Mug', + 'images' => [ + ['id' => 9, 'url' => '/m.jpg', 'hash' => 'secret', 'path' => '/fs', 'createdby' => 1], + ], + 'hash' => 'nope', + ], false, false, true), + 'images flag keeps allowlisted gallery rows' ); fwrite(STDOUT, "OK ProductCatalogServiceTest\n"); diff --git a/core/components/minishop3/tests/TokenMiddlewarePublicRoutesTest.php b/core/components/minishop3/tests/TokenMiddlewarePublicRoutesTest.php index cd04e6389..fe39388f9 100644 --- a/core/components/minishop3/tests/TokenMiddlewarePublicRoutesTest.php +++ b/core/components/minishop3/tests/TokenMiddlewarePublicRoutesTest.php @@ -35,6 +35,14 @@ $fail('cart/get must not be public — otherwise guest GET never auto-mints a token (#408)'); } +if (in_array('/api/v1/product/', $publicRoutes, true)) { + $fail('wide /api/v1/product/ prefix would publish the whole product group (#584)'); +} + +if (!str_contains($middlewareSrc, "'/api/v1/product/*/images'")) { + $fail('publicRoutePatterns must include /api/v1/product/*/images'); +} + foreach ( [ '/api/v1/product/get/', diff --git a/core/components/minishop3/tests/Unit/Middleware/TokenMiddlewarePublicPatternTest.php b/core/components/minishop3/tests/Unit/Middleware/TokenMiddlewarePublicPatternTest.php new file mode 100644 index 000000000..618608d35 --- /dev/null +++ b/core/components/minishop3/tests/Unit/Middleware/TokenMiddlewarePublicPatternTest.php @@ -0,0 +1,57 @@ +middleware = new TokenMiddleware(new modX()); + $this->isPublic = new ReflectionMethod(TokenMiddleware::class, 'isPublicRoute'); + $this->isPublic->setAccessible(true); + } + + protected function tearDown(): void + { + unset($_REQUEST['route']); + } + + #[DataProvider('routes')] + public function testPublicPatternDoesNotWidenProductGroup(string $route, bool $expected): void + { + $_REQUEST['route'] = $route; + + self::assertSame($expected, $this->isPublic->invoke($this->middleware, '/')); + } + + /** + * @return iterable + */ + public static function routes(): iterable + { + yield 'images' => ['/api/v1/product/42/images', true]; + yield 'images trailing slash' => ['/api/v1/product/42/images/', true]; + yield 'images query' => ['/api/v1/product/42/images?include_thumbs=1', true]; + yield 'filters prefix' => ['/api/v1/product/filters', true]; + yield 'list prefix' => ['/api/v1/product/list', true]; + yield 'unknown product sibling' => ['/api/v1/product/42/reviews', false]; + yield 'product root' => ['/api/v1/product/42', false]; + yield 'nested after images' => ['/api/v1/product/42/images/raw', false]; + } +} diff --git a/core/components/minishop3/tests/Unit/Services/Product/ProductGalleryPublicSerializerTest.php b/core/components/minishop3/tests/Unit/Services/Product/ProductGalleryPublicSerializerTest.php new file mode 100644 index 000000000..65c88ecf8 --- /dev/null +++ b/core/components/minishop3/tests/Unit/Services/Product/ProductGalleryPublicSerializerTest.php @@ -0,0 +1,120 @@ + 10, 'url' => '/a.jpg', 'name' => 'Front', 'description' => 'wide', 'position' => 0], + ['id' => 11, 'url' => '/b.jpg', 'name' => '', 'description' => '', 'position' => 1], + ['id' => 12, 'url' => '/c.jpg', 'name' => 'Side', 'description' => '', 'position' => 2], + ], + [ + 10 => ['thumb' => '/a_small.jpg', 'thumbs' => ['small' => '/a_small.jpg']], + 12 => ['thumb' => '/c_small.jpg', 'thumbs' => ['small' => '/c_small.jpg']], + ], + 'Kettle', + 12, + ); + + self::assertSame([10, 11, 12], array_column($items, 'id')); + self::assertSame('/a_small.jpg', $items[0]['thumb']); + self::assertSame(['small' => '/a_small.jpg'], $items[0]['thumbs']); + self::assertSame('/b.jpg', $items[1]['thumb']); + self::assertSame([], $items[1]['thumbs']); + self::assertTrue($items[2]['is_preview']); + self::assertFalse($items[0]['is_preview']); + self::assertSame('Front', $items[0]['alt']); + self::assertSame('Kettle', $items[1]['alt']); + self::assertSame('wide', $items[0]['description']); + } + + public function testStalePreviewFallsBackToFirstOriginal(): void + { + $items = ProductGalleryPublicSerializer::serializeGallery( + [ + ['id' => 10, 'url' => '/a.jpg', 'name' => 'A', 'position' => 0], + ['id' => 11, 'url' => '/b.jpg', 'name' => 'B', 'position' => 1], + ], + [], + 'Tea', + 99, + ); + + self::assertTrue($items[0]['is_preview']); + self::assertFalse($items[1]['is_preview']); + } + + public function testSerializeSkipsInvalidIdsAndDoesNotUseHash(): void + { + $items = ProductGalleryPublicSerializer::serializeGallery( + [ + ['id' => 0, 'url' => '/skip.jpg', 'hash' => 'abc', 'path' => '/secret'], + ['id' => 5, 'url' => '/ok.jpg', 'hash' => 'leak', 'path' => '/fs', 'createdby' => 3, 'name' => 'Ok'], + ], + [], + 'Tea', + 5, + ); + + self::assertCount(1, $items); + self::assertSame( + ['id', 'url', 'thumb', 'thumbs', 'name', 'description', 'alt', 'position', 'is_preview'], + array_keys($items[0]) + ); + self::assertArrayNotHasKey('hash', $items[0]); + self::assertArrayNotHasKey('path', $items[0]); + self::assertArrayNotHasKey('createdby', $items[0]); + self::assertTrue($items[0]['is_preview']); + } + + public function testEmptyOriginalsYieldEmptyGallery(): void + { + self::assertSame( + [], + ProductGalleryPublicSerializer::serializeGallery([], [], 'Tea', 0) + ); + } + + public function testSizeKeyFromChildUsesPathThenUrl(): void + { + self::assertSame('small', ProductGalleryPublicSerializer::sizeKeyFromChild('22/small/', '', 22)); + self::assertSame( + 'medium', + ProductGalleryPublicSerializer::sizeKeyFromChild('', '/assets/products/22/medium/a.jpg', 22) + ); + self::assertSame('', ProductGalleryPublicSerializer::sizeKeyFromChild('', '/a.jpg', 22)); + } + + public function testWhitelistItemsDropsInternalsAndNonArrays(): void + { + $clean = ProductGalleryPublicSerializer::whitelistItems([ + 'nope', + [ + 'id' => 1, + 'url' => '/x.jpg', + 'hash' => 'abc', + 'path' => '/fs', + 'createdby' => 9, + 'alt' => 'X', + 'thumbs' => ['small' => '/x_s.jpg', 'leak' => ['nope']], + ], + ]); + + self::assertCount(1, $clean); + self::assertSame(1, $clean[0]['id']); + self::assertSame('/x.jpg', $clean[0]['url']); + self::assertSame(['small' => '/x_s.jpg'], $clean[0]['thumbs']); + self::assertArrayNotHasKey('hash', $clean[0]); + self::assertArrayNotHasKey('path', $clean[0]); + self::assertArrayNotHasKey('createdby', $clean[0]); + } +} From 226b6161fe7b79ab3e3a6eb04c5775fa3776a21f Mon Sep 17 00:00:00 2001 From: Ibochkarev <2138260+Ibochkarev@users.noreply.github.com> Date: Sun, 6 Sep 2026 08:27:29 +0200 Subject: [PATCH 04/19] =?UTF-8?q?PR=20#600:=20feat(web-api):=20=D0=BD?= =?UTF-8?q?=D0=BE=D1=80=D0=BC=D0=B0=D0=BB=D0=B8=D0=B7=D0=BE=D0=B2=D0=B0?= =?UTF-8?q?=D1=82=D1=8C=20=D0=BA=D0=BE=D0=BD=D1=82=D1=80=D0=B0=D0=BA=D1=82?= =?UTF-8?q?=20=D0=BE=D1=82=D0=B2=D0=B5=D1=82=D0=B0=20=D0=BA=D0=BE=D1=80?= =?UTF-8?q?=D0=B7=D0=B8=D0=BD=D1=8B?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit https://github.com/modx-pro/MiniShop3/pull/600 --- .../minishop3/js/web/core/ApiClient.js | 10 +- .../minishop3/js/web/core/CartAPI.js | 20 +- .../Controllers/Api/Web/CartController.php | 15 ++ .../minishop3/src/ServiceRegistry.php | 4 + .../src/ServiceRegistryFactories.php | 1 + .../Services/Cart/CartResponseNormalizer.php | 224 ++++++++++++++++++ .../tests/CartResponseContractTest.php | 101 ++++++++ .../WebApi/Support/JourneyWebApiModx.php | 8 +- .../Cart/CartResponseNormalizerTest.php | 198 ++++++++++++++++ 9 files changed, 569 insertions(+), 12 deletions(-) create mode 100644 core/components/minishop3/src/Services/Cart/CartResponseNormalizer.php create mode 100644 core/components/minishop3/tests/CartResponseContractTest.php create mode 100644 core/components/minishop3/tests/Unit/Services/Cart/CartResponseNormalizerTest.php diff --git a/assets/components/minishop3/js/web/core/ApiClient.js b/assets/components/minishop3/js/web/core/ApiClient.js index 7ce58c765..085389283 100644 --- a/assets/components/minishop3/js/web/core/ApiClient.js +++ b/assets/components/minishop3/js/web/core/ApiClient.js @@ -34,8 +34,16 @@ class ApiClient { */ buildUrl (endpoint) { const url = new URL(this.baseUrl, window.location.origin) - url.searchParams.set('route', endpoint) + const qPos = endpoint.indexOf('?') + const path = qPos === -1 ? endpoint : endpoint.slice(0, qPos) + const query = qPos === -1 ? '' : endpoint.slice(qPos + 1) + url.searchParams.set('route', path) url.searchParams.set('ctx', this.ctx) + if (query !== '') { + new URLSearchParams(query).forEach((value, key) => { + url.searchParams.set(key, value) + }) + } return url } diff --git a/assets/components/minishop3/js/web/core/CartAPI.js b/assets/components/minishop3/js/web/core/CartAPI.js index c09882950..a5a60cfa3 100644 --- a/assets/components/minishop3/js/web/core/CartAPI.js +++ b/assets/components/minishop3/js/web/core/CartAPI.js @@ -9,8 +9,9 @@ * success: true/false, * message: "Message", * data: { - * cart: [], // Product array - * status: {}, // Cart totals (total_cost, total_count, etc.) + * cart: {}, // Legacy map keyed by product_key (empty object, not []) + * items: [], // Always an array of line items (preferred for Nuxt) + * status: {}, // Cart totals (total_cost, total_count, total_weight, total_discount, total_positions) * render: {} // HTML blocks for rendering (if requested) * } * } @@ -35,20 +36,19 @@ class CartAPI { * * GET /api/v1/cart/get * - * @param {Object} params - Additional parameters - * @param {Object} params.render - Render configuration (selectors for HTML update) - * @returns {Promise} - { success, message, data: { cart, status, render } } + * @param {Object} [params] - Query flags + * @param {boolean|number|string} [params.include_thumbs] - Opt-in item.thumb from product data + * @returns {Promise} - { success, message, data: { cart, items, status, render } } * * @example * const response = await cart.get() - * console.log(response.data.cart) + * console.log(response.data.items) * console.log(response.data.status.total_cost) */ async get (params = {}) { - const endpoint = '/api/v1/cart/get' - - if (params.render) { - // TODO: add render parameter support in backend + let endpoint = '/api/v1/cart/get' + if (params.include_thumbs) { + endpoint += '?include_thumbs=1' } return this.api.get(endpoint) diff --git a/core/components/minishop3/src/Controllers/Api/Web/CartController.php b/core/components/minishop3/src/Controllers/Api/Web/CartController.php index ebffe0f41..be9ba7a2c 100644 --- a/core/components/minishop3/src/Controllers/Api/Web/CartController.php +++ b/core/components/minishop3/src/Controllers/Api/Web/CartController.php @@ -7,6 +7,8 @@ use MiniShop3\Router\HttpStatus; use MiniShop3\Router\Response; use MiniShop3\Services\Api\WebApiContextResolver; +use MiniShop3\Services\Cart\CartResponseNormalizer; +use MiniShop3\Services\Catalog\CatalogQuery; use MODX\Revolution\modX; /** @@ -171,6 +173,10 @@ public function remove(array $params = []): Response * Get cart * GET /api/v1/cart/get * + * Query: include_thumbs (0|1, default 0). + * Response data: items (always array), cart (legacy map, empty object), status totals. + * Cart status is merchandise only. Delivery/payment/final: GET /api/v1/order/cost. + * * @param array $params URL parameters * @return Response */ @@ -264,6 +270,15 @@ protected function transformResponse(array $result): Response $input = $this->getRequestData(); $renderTokens = $input['render'] ?? null; + if (!empty($result['success']) && is_array($result['data'] ?? null)) { + /** @var CartResponseNormalizer $normalizer */ + $normalizer = $this->modx->services->get('ms3_cart_response_normalizer'); + $result['data'] = $normalizer->normalize( + $result['data'], + CatalogQuery::toBool($input['include_thumbs'] ?? false) + ); + } + if (!empty($renderTokens) && !empty($result['success'])) { $customerToken = $_REQUEST['ms3_token'] ?? ''; diff --git a/core/components/minishop3/src/ServiceRegistry.php b/core/components/minishop3/src/ServiceRegistry.php index c65eb80fe..f7a222d5d 100644 --- a/core/components/minishop3/src/ServiceRegistry.php +++ b/core/components/minishop3/src/ServiceRegistry.php @@ -301,6 +301,10 @@ class ServiceRegistry 'class' => \MiniShop3\Services\Cart\CartItemManager::class, 'interface' => null, ], + 'ms3_cart_response_normalizer' => [ + 'class' => \MiniShop3\Services\Cart\CartResponseNormalizer::class, + 'interface' => null, + ], 'ms3_cart_mutation_handler' => [ 'class' => \MiniShop3\Services\Cart\CartMutationHandler::class, 'interface' => null, diff --git a/core/components/minishop3/src/ServiceRegistryFactories.php b/core/components/minishop3/src/ServiceRegistryFactories.php index 074b51749..b01714c0f 100644 --- a/core/components/minishop3/src/ServiceRegistryFactories.php +++ b/core/components/minishop3/src/ServiceRegistryFactories.php @@ -99,6 +99,7 @@ public static function map(): array 'ms3_order_log' => $modxAndMs3(), 'ms3_manager_order_cost_recalculator' => $modxAndMs3(), 'ms3_cart_item_manager' => $modxAndMs3(), + 'ms3_cart_response_normalizer' => $modxOnly(), 'ms3_customer_address_manager' => $modxAndMs3(), 'ms3_customer_field_manager' => $modxAndMs3(), diff --git a/core/components/minishop3/src/Services/Cart/CartResponseNormalizer.php b/core/components/minishop3/src/Services/Cart/CartResponseNormalizer.php new file mode 100644 index 000000000..96b824441 --- /dev/null +++ b/core/components/minishop3/src/Services/Cart/CartResponseNormalizer.php @@ -0,0 +1,224 @@ + $data Domain cart payload (cart + status) + * @return array + */ + public function normalize(array $data, bool $includeThumbs = false): array + { + $cartMap = self::legacyCartMap($data['cart'] ?? []); + $items = self::projectItems($cartMap); + if ($includeThumbs) { + $items = $this->withThumbs($items); + } + + $data['cart'] = $cartMap === [] ? new \stdClass() : $cartMap; + $data['items'] = $items; + $data['status'] = self::projectStatus($data['status'] ?? []); + + return $data; + } + + /** + * @param mixed $cart + * @return array> + */ + private static function legacyCartMap(mixed $cart): array + { + if (!is_array($cart) || $cart === []) { + return []; + } + + $out = []; + if (array_is_list($cart)) { + foreach ($cart as $row) { + if (!is_array($row)) { + continue; + } + $key = (string) ($row['product_key'] ?? ''); + if ($key !== '') { + $out[$key] = $row; + } + } + + return $out; + } + + foreach ($cart as $key => $row) { + if (is_array($row)) { + $out[(string) $key] = $row; + } + } + + return $out; + } + + /** + * @param array> $cartMap + * @return list> + */ + private static function projectItems(array $cartMap): array + { + $keys = array_keys($cartMap); + usort( + $keys, + static function (string|int $a, string|int $b) use ($cartMap): int { + $idA = (int) ($cartMap[$a]['id'] ?? 0); + $idB = (int) ($cartMap[$b]['id'] ?? 0); + + return $idA <=> $idB ?: strcmp((string) $a, (string) $b); + } + ); + + $items = []; + foreach ($keys as $key) { + $items[] = self::projectItem((string) $key, $cartMap[$key]); + } + + return $items; + } + + /** + * @param array $raw + * @return array + */ + private static function projectItem(string $productKey, array $raw): array + { + $props = self::assoc($raw['properties'] ?? []); + $options = self::assoc($raw['options'] ?? []); + $count = (int) ($raw['count'] ?? 0); + $discountPrice = $props['discount_price'] ?? 0; + + return [ + 'product_key' => $productKey !== '' ? $productKey : (string) ($raw['product_key'] ?? ''), + 'product_id' => (int) ($raw['product_id'] ?? 0), + 'name' => (string) ($raw['name'] ?? ''), + 'count' => $count, + 'price' => self::money($raw['price'] ?? 0), + 'cost' => self::money($raw['cost'] ?? 0), + 'weight' => self::weight($raw['weight'] ?? 0), + 'options' => $options === [] ? new \stdClass() : $options, + 'old_price' => self::money($props['old_price'] ?? 0), + 'discount_price' => self::money($discountPrice), + 'discount_cost' => self::money($props['discount_cost'] ?? $discountPrice * $count), + ]; + } + + /** + * @param array $status + * @return array{ + * total_positions: int, + * total_count: int, + * total_cost: float, + * total_weight: float, + * total_discount: float + * } + */ + private static function projectStatus(array $status): array + { + return [ + 'total_positions' => (int) ($status['total_positions'] ?? 0), + 'total_count' => (int) ($status['total_count'] ?? 0), + 'total_cost' => self::money($status['total_cost'] ?? 0), + 'total_weight' => self::weight($status['total_weight'] ?? 0), + 'total_discount' => self::money($status['total_discount'] ?? 0), + ]; + } + + /** + * @param list> $items + * @return list> + */ + private function withThumbs(array $items): array + { + $ids = []; + foreach ($items as $item) { + $id = (int) ($item['product_id'] ?? 0); + if ($id > 0) { + $ids[$id] = $id; + } + } + + $urls = $this->lookupThumbs(array_values($ids)); + foreach ($items as $i => $item) { + $productId = (int) ($item['product_id'] ?? 0); + $url = trim($urls[$productId] ?? ''); + $items[$i]['thumb'] = $url !== '' ? $url : null; + } + + return $items; + } + + /** + * @param list $productIds + * @return array + */ + protected function lookupThumbs(array $productIds): array + { + if ($productIds === []) { + return []; + } + + $c = $this->modx->newQuery(msProductData::class); + $c->where(['id:IN' => $productIds]); + $c->select('id, thumb'); + + $out = []; + /** @var msProductData $row */ + foreach ($this->modx->getCollection(msProductData::class, $c) ?: [] as $row) { + $id = (int) $row->get('id'); + $url = trim((string) $row->get('thumb')); + if ($id > 0 && $url !== '') { + $out[$id] = $url; + } + } + + return $out; + } + + private static function money(mixed $value): float + { + return round((float) $value, self::MONEY_SCALE); + } + + private static function weight(mixed $value): float + { + return round((float) $value, self::WEIGHT_SCALE); + } + + /** + * @return array + */ + private static function assoc(mixed $value): array + { + if (is_string($value) && $value !== '') { + $decoded = json_decode($value, true); + $value = is_array($decoded) ? $decoded : []; + } + + return is_array($value) ? $value : []; + } +} diff --git a/core/components/minishop3/tests/CartResponseContractTest.php b/core/components/minishop3/tests/CartResponseContractTest.php new file mode 100644 index 000000000..54f573eef --- /dev/null +++ b/core/components/minishop3/tests/CartResponseContractTest.php @@ -0,0 +1,101 @@ +transformResponse($result);') < 6) { + $fail('every cart mutation and get must use transformResponse'); +} + +if (!str_contains($controller, 'ms3_cart_response_normalizer')) { + $fail('CartController must resolve ms3_cart_response_normalizer'); +} + +if (!str_contains($controller, 'include_thumbs')) { + $fail('CartController must document include_thumbs'); +} + +if (!str_contains($controller, 'order/cost')) { + $fail('CartController must document order/cost boundary'); +} + +if (str_contains($controller, 'OrderCostCalculator')) { + $fail('CartController must not call OrderCostCalculator'); +} + +if (!str_contains($normalizer, 'new \\stdClass()')) { + $fail('empty cart must encode as JSON object'); +} + +if (!str_contains($normalizer, "'items'")) { + $fail('normalizer must project items array'); +} + +if (!str_contains($controller, 'CatalogQuery::toBool')) { + $fail('CartController must parse include_thumbs at the HTTP boundary'); +} + +if (!str_contains($normalizer, 'bool $includeThumbs')) { + $fail('normalizer must take includeThumbs as bool, not a request bag'); +} + +if (str_contains($normalizer, 'OrderCostCalculator')) { + $fail('CartResponseNormalizer must not call OrderCostCalculator'); +} + +if (!str_contains($registry, 'ms3_cart_response_normalizer')) { + $fail('ServiceRegistry missing ms3_cart_response_normalizer'); +} + +if (!str_contains($factories, "'ms3_cart_response_normalizer'")) { + $fail('ServiceRegistryFactories missing ms3_cart_response_normalizer'); +} + +if (!str_contains($cartApi, 'items: []')) { + $fail('CartAPI must document items array'); +} + +if (!str_contains($cartApi, 'include_thumbs=1')) { + $fail('CartAPI.get must send include_thumbs'); +} + +if (!str_contains($apiClient, 'new URLSearchParams(query)')) { + $fail('ApiClient.buildUrl must copy endpoint query next to route, not inside route'); +} + +fwrite(STDOUT, "OK: Cart response contract checks passed\n"); +exit(0); diff --git a/core/components/minishop3/tests/Integration/WebApi/Support/JourneyWebApiModx.php b/core/components/minishop3/tests/Integration/WebApi/Support/JourneyWebApiModx.php index 84080568e..a4fc0deef 100644 --- a/core/components/minishop3/tests/Integration/WebApi/Support/JourneyWebApiModx.php +++ b/core/components/minishop3/tests/Integration/WebApi/Support/JourneyWebApiModx.php @@ -5,11 +5,12 @@ namespace MiniShop3\Tests\Integration\WebApi\Support; use MiniShop3\Model\msCustomer; +use MiniShop3\Services\Cart\CartResponseNormalizer; use MiniShop3\Tests\Stubs\ProcessorResponseStub; use MODX\Revolution\WebApiModxStub; /** - * WebApiModxStub extended with journey DI (ms3, catalog, customer orders). + * WebApiModxStub extended with journey DI (ms3, catalog, cart projection, customer orders). */ final class JourneyWebApiModx extends WebApiModxStub { @@ -21,6 +22,8 @@ final class JourneyWebApiModx extends WebApiModxStub public JourneyCustomerOrderService $customerOrders; + public CartResponseNormalizer $cartNormalizer; + /** @var array */ private array $options = []; @@ -42,6 +45,7 @@ public function get(string $field): string $this->tokenService = $this->journeyTokens; $this->catalog = new JourneyProductCatalog($this); $this->customerOrders = new JourneyCustomerOrderService($this); + $this->cartNormalizer = new CartResponseNormalizer($this); $rlPath = sys_get_temp_dir() . '/ms3-webapi-rl-' . getmypid(); if (!is_dir($rlPath)) { @@ -72,6 +76,7 @@ public function has(string $key): bool 'ms3_token_service', 'ms3_product_catalog', 'ms3_customer_order', + 'ms3_cart_response_normalizer', ], true); } @@ -82,6 +87,7 @@ public function get(string $key): mixed 'ms3_token_service' => $this->modx->tokenService, 'ms3_product_catalog' => $this->modx->catalog, 'ms3_customer_order' => $this->modx->customerOrders, + 'ms3_cart_response_normalizer' => $this->modx->cartNormalizer, default => null, }; } diff --git a/core/components/minishop3/tests/Unit/Services/Cart/CartResponseNormalizerTest.php b/core/components/minishop3/tests/Unit/Services/Cart/CartResponseNormalizerTest.php new file mode 100644 index 000000000..7e489f453 --- /dev/null +++ b/core/components/minishop3/tests/Unit/Services/Cart/CartResponseNormalizerTest.php @@ -0,0 +1,198 @@ +normalizer = new CartResponseNormalizer(new modX()); + } + + public function testEmptyCartIsObjectAndItemsAreArray(): void + { + $out = $this->normalizer->normalize([ + 'cart' => [], + 'status' => [], + ]); + + self::assertInstanceOf(\stdClass::class, $out['cart']); + self::assertSame('{}', json_encode($out['cart'])); + self::assertSame([], $out['items']); + self::assertSame('[]', json_encode($out['items'])); + self::assertSame(0, $out['status']['total_positions']); + self::assertSame(0, $out['status']['total_count']); + self::assertSame(0.0, $out['status']['total_cost']); + self::assertSame(0.0, $out['status']['total_weight']); + self::assertSame(0.0, $out['status']['total_discount']); + } + + public function testNonEmptyItemsMatchPositionsAndHideInternalFields(): void + { + $out = $this->normalizer->normalize([ + 'cart' => [ + 'ms-b' => [ + 'id' => 20, + 'order_id' => 99, + 'hash' => 'secret', + 'product_id' => 2, + 'name' => 'Later', + 'count' => 1, + 'price' => 50, + 'cost' => 50, + 'weight' => 0.1, + 'options' => [], + 'properties' => [], + ], + 'ms-a' => [ + 'id' => 10, + 'order_id' => 99, + 'product_id' => 1, + 'name' => 'First', + 'count' => 2, + 'price' => 100.456, + 'cost' => 200.456, + 'weight' => 0.1234, + 'options' => '{"color":"red"}', + 'properties' => [ + 'old_price' => 120, + 'discount_price' => 20, + 'discount_cost' => 40, + ], + ], + ], + 'status' => [ + 'total_positions' => 2, + 'total_count' => 3, + 'total_cost' => 250.456, + 'total_weight' => 0.3468, + 'total_discount' => 40, + ], + ]); + + self::assertCount(2, $out['items']); + self::assertSame(2, $out['status']['total_positions']); + self::assertSame('ms-a', $out['items'][0]['product_key']); + self::assertSame('ms-b', $out['items'][1]['product_key']); + self::assertSame(100.46, $out['items'][0]['price']); + self::assertSame(200.46, $out['items'][0]['cost']); + self::assertSame(0.123, $out['items'][0]['weight']); + self::assertSame(['color' => 'red'], $out['items'][0]['options']); + self::assertInstanceOf(\stdClass::class, $out['items'][1]['options']); + self::assertSame('{}', json_encode($out['items'][1]['options'])); + self::assertSame(120.0, $out['items'][0]['old_price']); + self::assertSame(20.0, $out['items'][0]['discount_price']); + self::assertSame(40.0, $out['items'][0]['discount_cost']); + self::assertArrayNotHasKey('thumb', $out['items'][0]); + self::assertArrayNotHasKey('order_id', $out['items'][0]); + self::assertArrayNotHasKey('hash', $out['items'][0]); + self::assertArrayNotHasKey('id', $out['items'][0]); + self::assertIsArray($out['cart']); + self::assertArrayHasKey('ms-a', $out['cart']); + self::assertSame(250.46, $out['status']['total_cost']); + self::assertSame(0.347, $out['status']['total_weight']); + self::assertSame(40.0, $out['status']['total_discount']); + } + + public function testDiscountCostFallsBackFromDiscountPriceTimesCount(): void + { + $out = $this->normalizer->normalize([ + 'cart' => [ + 'ms-x' => [ + 'product_id' => 3, + 'name' => 'Tea', + 'count' => 2, + 'price' => 100, + 'cost' => 200, + 'weight' => 0.2, + 'properties' => '{"old_price":120,"discount_price":20}', + ], + ], + 'status' => [], + ]); + + $item = $out['items'][0]; + self::assertSame(120.0, $item['old_price']); + self::assertSame(20.0, $item['discount_price']); + self::assertSame(40.0, $item['discount_cost']); + } + + public function testIncludeThumbsAddsUrlOrNull(): void + { + $normalizer = new class (new modX()) extends CartResponseNormalizer { + protected function lookupThumbs(array $productIds): array + { + return [12 => '/assets/small.jpg']; + } + }; + + $out = $normalizer->normalize( + [ + 'cart' => [ + 'ms-12' => [ + 'id' => 1, + 'product_id' => 12, + 'name' => 'Tea', + 'count' => 1, + 'price' => 10, + 'cost' => 10, + 'weight' => 0, + ], + 'ms-13' => [ + 'id' => 2, + 'product_id' => 13, + 'name' => 'Mug', + 'count' => 1, + 'price' => 5, + 'cost' => 5, + 'weight' => 0, + ], + ], + 'status' => ['total_positions' => 2, 'total_count' => 2], + ], + true + ); + + self::assertSame('/assets/small.jpg', $out['items'][0]['thumb']); + self::assertNull($out['items'][1]['thumb']); + } + + public function testWithoutThumbsFlagDoesNotQuery(): void + { + $normalizer = new class (new modX()) extends CartResponseNormalizer { + protected function lookupThumbs(array $productIds): array + { + throw new \RuntimeException('thumbs must not load without include_thumbs'); + } + }; + + $out = $normalizer->normalize([ + 'cart' => [ + 'ms-12' => [ + 'id' => 1, + 'product_id' => 12, + 'name' => 'Tea', + 'count' => 1, + 'price' => 10, + 'cost' => 10, + 'weight' => 0, + ], + ], + 'status' => ['total_positions' => 1, 'total_count' => 1], + ]); + + self::assertArrayNotHasKey('thumb', $out['items'][0]); + } +} From 5b498180305e33e1a6645f417c19ee52e319cd75 Mon Sep 17 00:00:00 2001 From: Ibochkarev <2138260+Ibochkarev@users.noreply.github.com> Date: Sun, 6 Sep 2026 08:27:29 +0200 Subject: [PATCH 05/19] =?UTF-8?q?PR=20#614:=20fix:=20=D0=BF=D0=BE=D0=BB?= =?UTF-8?q?=D0=B8=D1=82=D0=B8=D0=BA=D0=B0=20miniShopManagerPolicy=20=D0=B8?= =?UTF-8?q?=20ACL=20=D0=BF=D0=BE=D0=BB=D0=B5=D0=B9=20=D0=B7=D0=B0=D0=BA?= =?UTF-8?q?=D0=B0=D0=B7=D0=B0?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit https://github.com/modx-pro/MiniShop3/pull/614 --- _build/build.php | 71 +++++---- _build/elements/policies.php | 27 +--- _build/resolvers/resolver_09_policies.php | 79 +++++++++ .../config/manager_access_policy.php | 34 ++++ .../minishop3/config/routes/manager.php | 33 ++-- .../tests/ExtraFieldsRouteAclTest.php | 127 +++++++++++++++ .../tests/ModelFieldsRouteAclTest.php | 150 ++++++++++++++++++ .../minishop3/tests/PoliciesPackagingTest.php | 69 ++++++++ 8 files changed, 521 insertions(+), 69 deletions(-) create mode 100644 _build/resolvers/resolver_09_policies.php create mode 100644 core/components/minishop3/config/manager_access_policy.php create mode 100644 core/components/minishop3/tests/ExtraFieldsRouteAclTest.php create mode 100644 core/components/minishop3/tests/ModelFieldsRouteAclTest.php create mode 100644 core/components/minishop3/tests/PoliciesPackagingTest.php diff --git a/_build/build.php b/_build/build.php index cb4aa7110..ea4aee0e6 100644 --- a/_build/build.php +++ b/_build/build.php @@ -471,40 +471,15 @@ private function chunks(): void } /** - * Add access policy + * Access policies are nested under policy templates (see policyTemplates()). */ private function policies(): void { - /** @noinspection PhpIncludeInspection */ - $policies = include($this->config['elements'] . 'policies.php'); - if (!is_array($policies)) { - $this->modx->log(modX::LOG_LEVEL_ERROR, 'Could not package in Access Policies'); - return; - } - $attributes = [ - xPDOTransport::PRESERVE_KEYS => false, - xPDOTransport::UNIQUE_KEY => ['name'], - xPDOTransport::UPDATE_OBJECT => !empty($this->config['update']['policies']), - ]; - foreach ($policies as $name => $data) { - if (isset($data['data'])) { - $data['data'] = json_encode($data['data']); - } - /** @var $policy modAccessPolicy */ - $policy = $this->modx->newObject(modAccessPolicy::class); - $policy->fromArray(array_merge([ - 'name' => $name, - 'lexicon' => $this->config['name_lower'] . ':permissions', - ], $data) - , '', true, true); - $vehicle = $this->builder->createVehicle($policy, $attributes); - $this->builder->putVehicle($vehicle); - } - $this->modx->log(modX::LOG_LEVEL_INFO, 'Packaged in ' . count($policies) . ' Access Policies'); + $this->modx->log(modX::LOG_LEVEL_INFO, 'Access policies packaged via policyTemplates()'); } /** - * Add policy templates + * Add policy templates (and nested default policies). */ private function policyTemplates(): void { @@ -514,6 +489,11 @@ private function policyTemplates(): void $this->modx->log(modX::LOG_LEVEL_ERROR, 'Could not package in Policy Templates'); return; } + /** @noinspection PhpIncludeInspection */ + $policy_definitions = include($this->config['elements'] . 'policies.php'); + if (!is_array($policy_definitions)) { + $policy_definitions = []; + } $attributes = [ xPDOTransport::PRESERVE_KEYS => false, xPDOTransport::UNIQUE_KEY => ['name'], @@ -525,6 +505,11 @@ private function policyTemplates(): void xPDOTransport::UPDATE_OBJECT => !empty($this->config['update']['permission']), xPDOTransport::UNIQUE_KEY => ['template', 'name'], ], + 'Policies' => [ + xPDOTransport::PRESERVE_KEYS => false, + xPDOTransport::UPDATE_OBJECT => !empty($this->config['update']['policies']), + xPDOTransport::UNIQUE_KEY => ['name'], + ], ], ]; foreach ($policy_templates as $name => $data) { @@ -542,17 +527,37 @@ private function policyTemplates(): void $permissions[] = $permission; } } - /** @var $permission modAccessPolicyTemplate */ - $permission = $this->modx->newObject(modAccessPolicyTemplate::class); - $permission->fromArray(array_merge([ + /** @var modAccessPolicyTemplate $template */ + $template = $this->modx->newObject(modAccessPolicyTemplate::class); + $template->fromArray(array_merge([ 'name' => $name, 'lexicon' => $this->config['name_lower'] . ':permissions', ], $data) , '', true, true); if (!empty($permissions)) { - $permission->addMany($permissions); + $template->addMany($permissions); + } + if ($name === 'miniShopManagerPolicyTemplate' && $policy_definitions !== []) { + $policies = []; + foreach ($policy_definitions as $policyName => $policyData) { + $payload = $policyData; + if (isset($payload['data']) && is_array($payload['data'])) { + $payload['data'] = json_encode($payload['data']); + } + /** @var modAccessPolicy $policy */ + $policy = $this->modx->newObject(modAccessPolicy::class); + $policy->fromArray(array_merge([ + 'name' => $policyName, + 'lexicon' => $this->config['name_lower'] . ':permissions', + ], $payload) + , '', true, true); + $policies[] = $policy; + } + if ($policies !== []) { + $template->addMany($policies, 'Policies'); + } } - $vehicle = $this->builder->createVehicle($permission, $attributes); + $vehicle = $this->builder->createVehicle($template, $attributes); $this->builder->putVehicle($vehicle); } $this->modx->log(modX::LOG_LEVEL_INFO, 'Packaged in ' . count($policy_templates) . ' Access Policy Templates'); diff --git a/_build/elements/policies.php b/_build/elements/policies.php index f1a092703..cad37eba1 100644 --- a/_build/elements/policies.php +++ b/_build/elements/policies.php @@ -1,28 +1,5 @@ [ - 'description' => 'A policy for create and update MiniShop3 categories and products.', - 'parent' => 0, - 'class' => '', - 'lexicon' => 'minishop3:permissions', - 'data' => array_fill_keys($permissions, true), - ], -]; +return require dirname(__DIR__, 2) . '/core/components/minishop3/config/manager_access_policy.php'; diff --git a/_build/resolvers/resolver_09_policies.php b/_build/resolvers/resolver_09_policies.php new file mode 100644 index 000000000..d3eb5f143 --- /dev/null +++ b/_build/resolvers/resolver_09_policies.php @@ -0,0 +1,79 @@ + $options */ +if (!$transport->xpdo || !($transport instanceof xPDOTransport)) { + return false; +} + +$modx = $transport->xpdo; +$action = $options[xPDOTransport::PACKAGE_ACTION] ?? null; +if (!in_array($action, [xPDOTransport::ACTION_INSTALL, xPDOTransport::ACTION_UPGRADE], true)) { + return true; +} + +$definitionsFile = MODX_CORE_PATH . 'components/minishop3/config/manager_access_policy.php'; +if (!is_readable($definitionsFile)) { + $modx->log(modX::LOG_LEVEL_ERROR, '[MiniShop3] manager_access_policy.php not found'); + + return false; +} + +/** @var array> $definitions */ +$definitions = require $definitionsFile; +$template = $modx->getObject(modAccessPolicyTemplate::class, ['name' => 'miniShopManagerPolicyTemplate']); +if (!$template instanceof modAccessPolicyTemplate) { + $modx->log(modX::LOG_LEVEL_WARN, '[MiniShop3] miniShopManagerPolicyTemplate not found; skip policy repair'); + + return true; +} + +$templateId = (int) $template->get('id'); + +foreach ($definitions as $name => $data) { + /** @var modAccessPolicy|null $policy */ + $policy = $modx->getObject(modAccessPolicy::class, ['name' => $name]); + if ($policy instanceof modAccessPolicy) { + if ((int) $policy->get('template') !== $templateId) { + $policy->set('template', $templateId); + if (!$policy->save()) { + $modx->log(modX::LOG_LEVEL_ERROR, "[MiniShop3] Failed to link policy {$name} to template"); + + return false; + } + $modx->log(modX::LOG_LEVEL_INFO, "[MiniShop3] Linked existing policy {$name} to template"); + } + // Existing policy data is preserved (custom site ACL overrides). + continue; + } + + $payload = $data; + if (isset($payload['data']) && is_array($payload['data'])) { + $payload['data'] = json_encode($payload['data']); + } + + $policy = $modx->newObject(modAccessPolicy::class); + $policy->fromArray(array_merge([ + 'name' => $name, + 'lexicon' => 'minishop3:permissions', + 'template' => $templateId, + ], $payload), '', true, true); + + if ($policy->save()) { + $modx->log(modX::LOG_LEVEL_INFO, "[MiniShop3] Created access policy {$name}"); + continue; + } + + $modx->log(modX::LOG_LEVEL_ERROR, "[MiniShop3] Failed to create access policy {$name}"); + + return false; +} + +return true; diff --git a/core/components/minishop3/config/manager_access_policy.php b/core/components/minishop3/config/manager_access_policy.php new file mode 100644 index 000000000..7ad694fb0 --- /dev/null +++ b/core/components/minishop3/config/manager_access_policy.php @@ -0,0 +1,34 @@ + [ + 'description' => 'A policy for create and update MiniShop3 categories and products.', + 'parent' => 0, + 'class' => '', + 'lexicon' => 'minishop3:permissions', + 'data' => array_fill_keys($permissions, true), + ], +]; diff --git a/core/components/minishop3/config/routes/manager.php b/core/components/minishop3/config/routes/manager.php index b86435845..28066761a 100644 --- a/core/components/minishop3/config/routes/manager.php +++ b/core/components/minishop3/config/routes/manager.php @@ -189,6 +189,7 @@ new PermissionMiddleware($modx, 'view_document') ]); + // Extra fields reads: order/product forms load schema without settings perm (#613) $router->group('/extra-fields', function ($router) use ($modx) { $router->get('', function ($params) use ($modx) { $controller = new \MiniShop3\Controllers\Api\Manager\ExtraFieldsController($modx); @@ -198,6 +199,10 @@ $controller = new \MiniShop3\Controllers\Api\Manager\ExtraFieldsController($modx); return $controller->get($params); }); + }); + + // Extra fields writes: schema mutations require mssetting_save (#381) + $router->group('/extra-fields', function ($router) use ($modx) { $router->post('', function ($params) use ($modx) { $controller = new \MiniShop3\Controllers\Api\Manager\ExtraFieldsController($modx); return $controller->create(); @@ -984,6 +989,7 @@ new PermissionMiddleware($modx, 'mssetting_save') ]); + // Model fields reads: order/product forms load schema without settings perm (#613) $router->group('/model-fields', function ($router) use ($modx) { $router->get('/models', function ($params) use ($modx) { $controller = new \MiniShop3\Controllers\Api\Manager\ModelFieldsController($modx); @@ -1009,6 +1015,22 @@ $controller = new \MiniShop3\Controllers\Api\Manager\ModelFieldsController($modx); return $controller->getSections($params); }); + + // Field routes + $router->get('', function ($params) use ($modx) { + $allParams = array_merge($_GET, $params); + + $controller = new \MiniShop3\Controllers\Api\Manager\ModelFieldsController($modx); + return $controller->getList($allParams); + }); + $router->get('/{id}', function ($params) use ($modx) { + $controller = new \MiniShop3\Controllers\Api\Manager\ModelFieldsController($modx); + return $controller->get($params); + }); + }); + + // Model fields writes: schema mutations require mssetting_save (#381) + $router->group('/model-fields', function ($router) use ($modx) { $router->post('/sections', function ($params) use ($modx) { $input = file_get_contents('php://input'); $data = json_decode($input, true) ?: []; @@ -1036,17 +1058,6 @@ return $controller->deleteSection($params); }); - // Field routes - $router->get('', function ($params) use ($modx) { - $allParams = array_merge($_GET, $params); - - $controller = new \MiniShop3\Controllers\Api\Manager\ModelFieldsController($modx); - return $controller->getList($allParams); - }); - $router->get('/{id}', function ($params) use ($modx) { - $controller = new \MiniShop3\Controllers\Api\Manager\ModelFieldsController($modx); - return $controller->get($params); - }); $router->post('', function ($params) use ($modx) { $input = file_get_contents('php://input'); $data = json_decode($input, true) ?: []; diff --git a/core/components/minishop3/tests/ExtraFieldsRouteAclTest.php b/core/components/minishop3/tests/ExtraFieldsRouteAclTest.php new file mode 100644 index 000000000..e39ad9608 --- /dev/null +++ b/core/components/minishop3/tests/ExtraFieldsRouteAclTest.php @@ -0,0 +1,127 @@ + + */ +$extractRouteGroups = static function (string $src, string $path): array { + $needle = "\$router->group('{$path}'"; + $groups = []; + $offset = 0; + $len = strlen($src); + + while (($pos = strpos($src, $needle, $offset)) !== false) { + $open = strpos($src, 'function ($router)', $pos); + if ($open === false) { + break; + } + $braceStart = strpos($src, '{', $open); + if ($braceStart === false) { + break; + } + + $depth = 0; + $closedAt = null; + for ($i = $braceStart; $i < $len; $i++) { + $ch = $src[$i]; + if ($ch === '{') { + $depth++; + } elseif ($ch === '}') { + $depth--; + if ($depth === 0) { + $closedAt = $i; + break; + } + } + } + + if ($closedAt === null) { + break; + } + + $body = substr($src, $braceStart + 1, $closedAt - $braceStart - 1); + $perm = null; + if ( + preg_match( + '/^\}\s*,\s*\[\s*new\s+PermissionMiddleware\(\s*\$modx\s*,\s*\'([^\']+)\'\s*\)/', + substr($src, $closedAt, 200), + $permMatch + ) + ) { + $perm = $permMatch[1]; + } + + $groups[] = ['body' => $body, 'perm' => $perm]; + $offset = $closedAt + 1; + } + + return $groups; +}; + +$routesFile = dirname(__DIR__) . '/config/routes/manager.php'; +$src = file_get_contents($routesFile); +if ($src === false || $src === '') { + $fail('cannot read manager.php routes'); +} + +$groups = $extractRouteGroups($src, '/extra-fields'); +if (count($groups) !== 2) { + $fail('expected two /extra-fields route groups (read + write), got ' . count($groups)); +} + +/** @var array $routePerm */ +$routePerm = []; + +foreach ($groups as $group) { + if ( + preg_match_all( + "/\\\$router->(get|put|post|delete)\(\s*'([^']*)'/", + $group['body'], + $routeMatches, + PREG_SET_ORDER + ) + ) { + foreach ($routeMatches as $routeMatch) { + $key = strtoupper($routeMatch[1]) . ' ' . $routeMatch[2]; + $routePerm[$key] = $group['perm']; + } + } +} + +$expected = [ + 'GET ' => null, + 'GET /{id}' => null, + 'POST ' => 'mssetting_save', + 'PUT /{id}' => 'mssetting_save', + 'DELETE /{id}' => 'mssetting_save', +]; + +foreach ($expected as $key => $perm) { + if (!array_key_exists($key, $routePerm)) { + $fail("missing route {$key}"); + } + if ($routePerm[$key] !== $perm) { + $fail("{$key} ACL mismatch"); + } +} + +foreach ($routePerm as $key => $perm) { + if (!array_key_exists($key, $expected)) { + $fail("unexpected extra-fields route registered: {$key}"); + } +} + +fwrite(STDOUT, "OK ExtraFieldsRouteAclTest\n"); +exit(0); diff --git a/core/components/minishop3/tests/ModelFieldsRouteAclTest.php b/core/components/minishop3/tests/ModelFieldsRouteAclTest.php new file mode 100644 index 000000000..8d26e1d9f --- /dev/null +++ b/core/components/minishop3/tests/ModelFieldsRouteAclTest.php @@ -0,0 +1,150 @@ + + */ +$extractRouteGroups = static function (string $src, string $path): array { + $needle = "\$router->group('{$path}'"; + $groups = []; + $offset = 0; + $len = strlen($src); + + while (($pos = strpos($src, $needle, $offset)) !== false) { + $open = strpos($src, 'function ($router)', $pos); + if ($open === false) { + break; + } + $braceStart = strpos($src, '{', $open); + if ($braceStart === false) { + break; + } + + $depth = 0; + $closedAt = null; + for ($i = $braceStart; $i < $len; $i++) { + $ch = $src[$i]; + if ($ch === '{') { + $depth++; + } elseif ($ch === '}') { + $depth--; + if ($depth === 0) { + $closedAt = $i; + break; + } + } + } + + if ($closedAt === null) { + break; + } + + $body = substr($src, $braceStart + 1, $closedAt - $braceStart - 1); + $perm = null; + if ( + preg_match( + '/^\}\s*,\s*\[\s*new\s+PermissionMiddleware\(\s*\$modx\s*,\s*\'([^\']+)\'\s*\)/', + substr($src, $closedAt, 200), + $permMatch + ) + ) { + $perm = $permMatch[1]; + } + + $groups[] = ['body' => $body, 'perm' => $perm]; + $offset = $closedAt + 1; + } + + return $groups; +}; + +$routesFile = dirname(__DIR__) . '/config/routes/manager.php'; +$src = file_get_contents($routesFile); +if ($src === false || $src === '') { + $fail('cannot read manager.php routes'); +} + +$groups = $extractRouteGroups($src, '/model-fields'); +if ($groups === []) { + $fail('no /model-fields route groups found'); +} + +/** @var array $routePerm */ +$routePerm = []; + +foreach ($groups as $group) { + if ( + preg_match_all( + "/\\\$router->(get|put|post|delete)\(\s*'([^']*)'/", + $group['body'], + $routeMatches, + PREG_SET_ORDER + ) + ) { + foreach ($routeMatches as $routeMatch) { + $key = strtoupper($routeMatch[1]) . ' ' . $routeMatch[2]; + if (array_key_exists($key, $routePerm) && $routePerm[$key] !== $group['perm']) { + $fail("conflicting permissions for {$key}"); + } + $routePerm[$key] = $group['perm']; + } + } +} + +$expectedReads = [ + 'GET /models', + 'GET /visible/{model}', + 'GET /combo-options/{model}', + 'GET /combo-options/{model}/{field_name}', + 'GET /sections/{model}', + 'GET ', + 'GET /{id}', +]; + +$expectedWrites = [ + 'POST /sections' => 'mssetting_save', + 'PUT /sections/ranks' => 'mssetting_save', + 'PUT /sections/{id}' => 'mssetting_save', + 'DELETE /sections/{id}' => 'mssetting_save', + 'POST ' => 'mssetting_save', + 'PUT /ranks' => 'mssetting_save', + 'PUT /{id}' => 'mssetting_save', + 'DELETE /{id}' => 'mssetting_save', +]; + +foreach ($expectedReads as $key) { + if (!array_key_exists($key, $routePerm)) { + $fail("missing read route {$key}"); + } + if ($routePerm[$key] !== null) { + $fail("{$key} must be auth-only, got: " . var_export($routePerm[$key], true)); + } +} + +foreach ($expectedWrites as $key => $perm) { + if (($routePerm[$key] ?? null) !== $perm) { + $fail("{$key} must require {$perm}, got: " . var_export($routePerm[$key] ?? null, true)); + } +} + +$expectedKeys = array_merge($expectedReads, array_keys($expectedWrites)); +foreach ($routePerm as $key => $perm) { + if (!in_array($key, $expectedKeys, true)) { + $fail("unexpected model-fields route registered: {$key}"); + } +} + +fwrite(STDOUT, "OK ModelFieldsRouteAclTest\n"); +exit(0); diff --git a/core/components/minishop3/tests/PoliciesPackagingTest.php b/core/components/minishop3/tests/PoliciesPackagingTest.php new file mode 100644 index 000000000..8f5cf8366 --- /dev/null +++ b/core/components/minishop3/tests/PoliciesPackagingTest.php @@ -0,0 +1,69 @@ + [")) { + $fail('policyTemplates() must declare Policies related object attributes'); +} + +if (!str_contains($buildSrc, "addMany(\$policies, 'Policies')")) { + $fail('miniShopManagerPolicy must be nested under miniShopManagerPolicyTemplate'); +} + +if (!preg_match('/private function policies\(\): void\s*\{[^}]*Packaged via policyTemplates/s', $buildSrc)) { + if (!str_contains($buildSrc, 'Access policies packaged via policyTemplates()')) { + $fail('standalone policies() must be no-op with packaging note'); + } +} + +$resolverPath = $repoRoot . '/_build/resolvers/resolver_09_policies.php'; +if (!is_readable($resolverPath)) { + $fail('resolver_09_policies.php missing'); +} + +$resolverSrc = file_get_contents($resolverPath); +if ($resolverSrc === false || !str_contains($resolverSrc, 'manager_access_policy.php')) { + $fail('resolver must load manager_access_policy.php'); +} + +$policyConfig = $repoRoot . '/core/components/minishop3/config/manager_access_policy.php'; +if (!is_readable($policyConfig)) { + $fail('manager_access_policy.php missing'); +} + +/** @var array> $definitions */ +$definitions = require $policyConfig; +if (!isset($definitions['miniShopManagerPolicy']['data']['msorder_save'])) { + $fail('miniShopManagerPolicy must grant msorder_save'); +} +if (!isset($definitions['miniShopManagerPolicy']['data']['mssetting_save'])) { + $fail('miniShopManagerPolicy must grant mssetting_save'); +} + +$elementsPolicies = $repoRoot . '/_build/elements/policies.php'; +$elementsSrc = file_get_contents($elementsPolicies); +if ($elementsSrc === false || !str_contains($elementsSrc, 'manager_access_policy.php')) { + $fail('_build/elements/policies.php must require manager_access_policy.php'); +} + +fwrite(STDOUT, "OK PoliciesPackagingTest\n"); +exit(0); From 62087989a909f278a227a465d634c0cd13b687fc Mon Sep 17 00:00:00 2001 From: Ibochkarev <2138260+Ibochkarev@users.noreply.github.com> Date: Sun, 6 Sep 2026 08:27:29 +0200 Subject: [PATCH 06/19] =?UTF-8?q?PR=20#619:=20fix(mgr):=20UTF-8=20whitespa?= =?UTF-8?q?ce=20=D0=B2=20combo=20labels=20model-fields?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit https://github.com/modx-pro/MiniShop3/pull/619 --- .../src/Services/ComboConfigManager.php | 7 +- .../ComboConfigManagerBuildLabelTest.php | 78 +++++++++++++++++++ 2 files changed, 83 insertions(+), 2 deletions(-) create mode 100644 core/components/minishop3/tests/Unit/Services/ComboConfigManagerBuildLabelTest.php diff --git a/core/components/minishop3/src/Services/ComboConfigManager.php b/core/components/minishop3/src/Services/ComboConfigManager.php index fd43506fb..2026010b4 100644 --- a/core/components/minishop3/src/Services/ComboConfigManager.php +++ b/core/components/minishop3/src/Services/ComboConfigManager.php @@ -318,8 +318,11 @@ function ($matches) use ($item) { }, $template ); - // Clean up multiple spaces and trim - $label = trim(preg_replace('/\s+/', ' ', $label)); + // Collapse Unicode whitespace (e.g. NBSP U+00A0). Without /u, PCRE treats the + // subject as bytes and does not treat UTF-8 NBSP as \s, so combo labels can keep + // non-ASCII spaces and break downstream JSON consumers of model-fields (#618). + $collapsed = preg_replace('/\s+/u', ' ', $label); + $label = trim(is_string($collapsed) ? $collapsed : $label); } else { // Fallback to single field $label = $item->get($singleField); diff --git a/core/components/minishop3/tests/Unit/Services/ComboConfigManagerBuildLabelTest.php b/core/components/minishop3/tests/Unit/Services/ComboConfigManagerBuildLabelTest.php new file mode 100644 index 000000000..203ad81f0 --- /dev/null +++ b/core/components/minishop3/tests/Unit/Services/ComboConfigManagerBuildLabelTest.php @@ -0,0 +1,78 @@ +invokeBuildLabel( + ['first_name' => 'Руслан', 'last_name' => 'Иванов'], + '{first_name} {last_name}', + 'name' + ); + + self::assertSame('Руслан Иванов', $label); + self::assertTrue(mb_check_encoding($label, 'UTF-8')); + self::assertNotFalse(json_encode(['label' => $label], JSON_THROW_ON_ERROR)); + } + + public function testUnicodeNbspIsCollapsedToSingleSpace(): void + { + $nbsp = "\u{00A0}"; + $label = $this->invokeBuildLabel( + ['first_name' => 'Анна', 'last_name' => 'Петрова'], + '{first_name}' . $nbsp . $nbsp . '{last_name}', + 'name' + ); + + self::assertSame('Анна Петрова', $label); + self::assertTrue(mb_check_encoding($label, 'UTF-8')); + self::assertNotFalse(json_encode(['label' => $label], JSON_THROW_ON_ERROR)); + } + + /** + * @param array $fields + */ + private function invokeBuildLabel(array $fields, ?string $template, string $singleField): string + { + $item = new class ($fields) { + /** @param array $fields */ + public function __construct(private array $fields) + { + } + + public function get(string $key): mixed + { + return $this->fields[$key] ?? null; + } + }; + + $manager = new ComboConfigManager(new modX()); + $method = new ReflectionMethod(ComboConfigManager::class, 'buildLabel'); + $method->setAccessible(true); + + return (string) $method->invoke($manager, $item, $template, $singleField); + } +} From 586ca3b50d2aefc18e6a59e3af05061d93d8af19 Mon Sep 17 00:00:00 2001 From: Ibochkarev <2138260+Ibochkarev@users.noreply.github.com> Date: Sun, 6 Sep 2026 08:27:29 +0200 Subject: [PATCH 07/19] =?UTF-8?q?PR=20#620:=20fix(mgr):=20=D1=81=D0=BE?= =?UTF-8?q?=D1=80=D1=82=D0=B8=D1=80=D0=BE=D0=B2=D0=BA=D0=B0=20=D1=81=D0=B5?= =?UTF-8?q?=D0=BA=D1=86=D0=B8=D0=B9=20=D0=BD=D0=B0=20=D1=81=D1=82=D1=80?= =?UTF-8?q?=D0=B0=D0=BD=D0=B8=D1=86=D0=B5=20=D1=82=D0=BE=D0=B2=D0=B0=D1=80?= =?UTF-8?q?=D0=B0=20=D0=BF=D0=BE=20sort=5Forder?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit https://github.com/modx-pro/MiniShop3/pull/620 --- .../src/components/ProductDataFields.vue | 34 +++-------- .../src/utils/groupProductDataSections.js | 55 +++++++++++++++++ .../utils/groupProductDataSections.test.js | 59 +++++++++++++++++++ 3 files changed, 121 insertions(+), 27 deletions(-) create mode 100644 vueManager/src/utils/groupProductDataSections.js create mode 100644 vueManager/src/utils/groupProductDataSections.test.js diff --git a/vueManager/src/components/ProductDataFields.vue b/vueManager/src/components/ProductDataFields.vue index 473db07e7..946238239 100644 --- a/vueManager/src/components/ProductDataFields.vue +++ b/vueManager/src/components/ProductDataFields.vue @@ -7,6 +7,7 @@ import { useToast } from 'primevue/usetoast' import { computed, onMounted, ref } from 'vue' import request from '../request.js' +import { groupProductDataSections } from '../utils/groupProductDataSections.js' import { isFullWidthExtraFieldXtype, parseStructuredExtraFieldValue, @@ -201,32 +202,11 @@ const visibleFields = computed(() => { }) /** - * Group fields by sections - * Show only !hidden sections + * Group fields by sections (array ordered by section.sort_order). + * Do not return an id-keyed object: Vue/JS enumerates those keys by ascending id (#611). */ const fieldsBySections = computed(() => { - const sections = {} - - visibleFields.value.forEach(field => { - const sectionKey = field.section || 'default' - const sectionConfig = fieldsConfig.value.sections[sectionKey] - - // Skip hidden sections - if (sectionConfig && sectionConfig.hidden === true) { - return - } - - if (!sections[sectionKey]) { - sections[sectionKey] = { - ...sectionConfig, - fields: [], - } - } - - sections[sectionKey].fields.push(field) - }) - - return sections + return groupProductDataSections(visibleFields.value, fieldsConfig.value.sections || {}) }) // Load configuration on mount @@ -253,9 +233,9 @@ onMounted(() => {
} fields Visible field configs (already filtered). + * @param {Object} sectionsById Map from section id → section config. + * @returns {Array<{ key: string|number, fields: Array, [string]: any }>} + */ +export function groupProductDataSections(fields, sectionsById = {}) { + const grouped = new Map() + + for (const field of fields) { + // Normalize to string so Map keys match JSON object keys from getPageFields. + const sectionKey = field.section == null || field.section === '' ? 'default' : String(field.section) + const sectionConfig = sectionsById[sectionKey] + + if (sectionConfig?.hidden === true) { + continue + } + + if (!grouped.has(sectionKey)) { + grouped.set(sectionKey, { + // Fallback identity when section is missing from the map; API `key` (section_key) wins via spread. + key: sectionKey, + ...(sectionConfig || {}), + fields: [], + }) + } + + grouped.get(sectionKey).fields.push(field) + } + + const sections = Array.from(grouped.values()) + + sections.sort((a, b) => { + const sortA = Number.isFinite(Number(a.sort_order)) ? Number(a.sort_order) : Number.MAX_SAFE_INTEGER + const sortB = Number.isFinite(Number(b.sort_order)) ? Number(b.sort_order) : Number.MAX_SAFE_INTEGER + if (sortA !== sortB) { + return sortA - sortB + } + + const idA = Number(a.id ?? a.key) + const idB = Number(b.id ?? b.key) + if (Number.isFinite(idA) && Number.isFinite(idB) && idA !== idB) { + return idA - idB + } + + return String(a.id ?? a.key).localeCompare(String(b.id ?? b.key)) + }) + + return sections +} diff --git a/vueManager/src/utils/groupProductDataSections.test.js b/vueManager/src/utils/groupProductDataSections.test.js new file mode 100644 index 000000000..68e04fdcd --- /dev/null +++ b/vueManager/src/utils/groupProductDataSections.test.js @@ -0,0 +1,59 @@ +import { describe, expect, it } from 'vitest' + +import { groupProductDataSections } from './groupProductDataSections.js' + +describe('groupProductDataSections', () => { + it('orders sections by sort_order even when section ids ascend differently', () => { + const sectionsById = { + 1: { id: 1, key: 'main', sort_order: 20, label: 'Main' }, + 3: { id: 3, key: 'first', sort_order: 10, label: 'First' }, + 5: { id: 5, key: 'currency', sort_order: 15, label: 'Currency' }, + } + const fields = [ + { name: 'article', section: 1 }, + { name: 'price', section: 3 }, + { name: 'currency', section: 5 }, + ] + + const result = groupProductDataSections(fields, sectionsById) + + expect(result.map(s => s.id)).toEqual([3, 5, 1]) + expect(result.map(s => s.key)).toEqual(['first', 'currency', 'main']) + expect(result.map(s => s.label)).toEqual(['First', 'Currency', 'Main']) + }) + + it('skips hidden sections', () => { + const sectionsById = { + 1: { id: 1, key: 'main', sort_order: 10, hidden: false }, + 2: { id: 2, key: 'hidden', sort_order: 5, hidden: true }, + } + const fields = [ + { name: 'a', section: 1 }, + { name: 'b', section: 2 }, + ] + + expect(groupProductDataSections(fields, sectionsById).map(s => s.id)).toEqual([1]) + }) + + it('keeps fields inside a section in input order', () => { + const sectionsById = { + 1: { id: 1, sort_order: 10 }, + } + const fields = [ + { name: 'second', section: 1, sort_order: 20 }, + { name: 'first', section: 1, sort_order: 10 }, + ] + + expect(groupProductDataSections(fields, sectionsById)[0].fields.map(f => f.name)).toEqual([ + 'second', + 'first', + ]) + }) + + it('falls back to default section when field.section is missing', () => { + const result = groupProductDataSections([{ name: 'orphan' }], {}) + expect(result).toHaveLength(1) + expect(result[0].key).toBe('default') + expect(result[0].fields[0].name).toBe('orphan') + }) +}) From f583aee9ed72f882bdd9a2fb62288ee277e94bee Mon Sep 17 00:00:00 2001 From: Ibochkarev <2138260+Ibochkarev@users.noreply.github.com> Date: Sun, 6 Sep 2026 08:27:29 +0200 Subject: [PATCH 08/19] =?UTF-8?q?PR=20#623:=20feat(mgr):=20=D1=82=D0=B8?= =?UTF-8?q?=D0=BF=20=D0=94=D0=B0=D1=82=D0=B0=20=D0=B4=D0=BB=D1=8F=20extra?= =?UTF-8?q?=20fields?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit https://github.com/modx-pro/MiniShop3/pull/623 --- .../minishop3/lexicon/en/vue.inc.php | 2 + .../minishop3/lexicon/ru/vue.inc.php | 2 + vueManager/src/components/DynamicField.vue | 97 +++++++++++++++---- .../src/components/ExtraFieldsManager.vue | 19 ++-- vueManager/src/utils/structuredExtraField.js | 35 ++++++- .../src/utils/structuredExtraField.test.js | 42 ++++++++ 6 files changed, 169 insertions(+), 28 deletions(-) create mode 100644 vueManager/src/utils/structuredExtraField.test.js diff --git a/core/components/minishop3/lexicon/en/vue.inc.php b/core/components/minishop3/lexicon/en/vue.inc.php index 3e59cca7d..14e504192 100644 --- a/core/components/minishop3/lexicon/en/vue.inc.php +++ b/core/components/minishop3/lexicon/en/vue.inc.php @@ -165,6 +165,7 @@ $_lang['ms3_vue_xtype_combo_vendor'] = 'Vendor (combo)'; $_lang['ms3_vue_xtype_combo_autocomplete'] = 'Autocomplete (combo)'; $_lang['ms3_vue_xtype_combo_options'] = 'Product Options (chips)'; +$_lang['ms3_vue_xtype_datefield'] = 'Date'; // Dropdown list settings $_lang['ms3_vue_select_options_label'] = 'List Options'; @@ -215,6 +216,7 @@ $_lang['ms3_vue_dbtype_text'] = 'TEXT (text)'; $_lang['ms3_vue_dbtype_int'] = 'INT (integer)'; $_lang['ms3_vue_dbtype_decimal'] = 'DECIMAL (decimal)'; +$_lang['ms3_vue_dbtype_date'] = 'DATE (date only)'; $_lang['ms3_vue_dbtype_datetime'] = 'DATETIME (date and time)'; $_lang['ms3_vue_dbtype_timestamp'] = 'TIMESTAMP'; $_lang['ms3_vue_dbtype_tinyint'] = 'TINYINT (0/1)'; diff --git a/core/components/minishop3/lexicon/ru/vue.inc.php b/core/components/minishop3/lexicon/ru/vue.inc.php index 64d600371..6c8705e36 100644 --- a/core/components/minishop3/lexicon/ru/vue.inc.php +++ b/core/components/minishop3/lexicon/ru/vue.inc.php @@ -165,6 +165,7 @@ $_lang['ms3_vue_xtype_combo_vendor'] = 'Производитель (combo)'; $_lang['ms3_vue_xtype_combo_autocomplete'] = 'Автодополнение (combo)'; $_lang['ms3_vue_xtype_combo_options'] = 'Опции товара (chips)'; +$_lang['ms3_vue_xtype_datefield'] = 'Дата'; // Настройки выпадающего списка $_lang['ms3_vue_select_options_label'] = 'Варианты списка'; @@ -215,6 +216,7 @@ $_lang['ms3_vue_dbtype_text'] = 'TEXT (текст)'; $_lang['ms3_vue_dbtype_int'] = 'INT (целое число)'; $_lang['ms3_vue_dbtype_decimal'] = 'DECIMAL (число с точностью)'; +$_lang['ms3_vue_dbtype_date'] = 'DATE (только дата)'; $_lang['ms3_vue_dbtype_datetime'] = 'DATETIME (дата и время)'; $_lang['ms3_vue_dbtype_timestamp'] = 'TIMESTAMP'; $_lang['ms3_vue_dbtype_tinyint'] = 'TINYINT (0/1)'; diff --git a/vueManager/src/components/DynamicField.vue b/vueManager/src/components/DynamicField.vue index 8cb5e2883..2d063586a 100644 --- a/vueManager/src/components/DynamicField.vue +++ b/vueManager/src/components/DynamicField.vue @@ -92,19 +92,25 @@ /> - + Unknown field type: {{ fieldConfig.xtype }} - - + @@ -256,9 +261,10 @@ import Textarea from 'primevue/textarea' import ToggleSwitch from 'primevue/toggleswitch' import { computed, ref, watch } from 'vue' +import { formatLocalDateYmd } from '../utils/formatLocalDateYmd.js' import { getKeyValueConfigFromField, serializeKeyValueForPost } from '../utils/keyValueField.js' import { getRepeaterConfigFromField } from '../utils/repeaterField.js' -import { parseStructuredExtraFieldValue } from '../utils/structuredExtraField.js' +import { DATEFIELD_XTYPE, parseDateFieldValue, parseStructuredExtraFieldValue } from '../utils/structuredExtraField.js' import AutocompleteCombo from './AutocompleteCombo.vue' import FileBrowser from './FileBrowser.vue' import KeyValueField from './KeyValueField.vue' @@ -332,7 +338,7 @@ const isFileBrowserXtype = computed(() => { * Determine if field is complex type (requires hidden field with JSON) */ const isComplexField = computed(() => { - const complexTypes = ['combobox', 'datefield', 'colorpicker', 'chips', 'multiselect'] + const complexTypes = ['combobox', 'colorpicker', 'chips', 'multiselect'] return complexTypes.includes(props.fieldConfig.xtype) }) @@ -368,11 +374,28 @@ const selectOptions = computed(() => { const repeaterConfig = computed(() => getRepeaterConfigFromField(props.fieldConfig)) const keyValueConfig = computed(() => getKeyValueConfigFromField(props.fieldConfig)) +const isDateField = computed(() => props.fieldConfig.xtype === DATEFIELD_XTYPE) function normalizeIncomingValue(value) { return parseStructuredExtraFieldValue(props.fieldConfig.xtype, value) } +function normalizedDateString(value) { + if (value == null || value === '') { + return null + } + + if (typeof value === 'string') { + return value.match(/^(\d{4}-\d{2}-\d{2})/)?.[1] ?? value + } + + return formatLocalDateYmd(value) +} + +function sameCalendarDay(left, right) { + return normalizedDateString(left) === normalizedDateString(right) +} + /** * Serialise the repeater value for the hidden legacy-form input. * RepeaterField emits an array; the processor expects JSON string or array. @@ -426,27 +449,61 @@ const serializedValue = computed(() => { const emit = defineEmits(['update:modelValue', 'blur']) -// Local value for v-model -const localValue = ref(normalizeIncomingValue(props.modelValue)) +// Local value for v-model (non-date fields) +const localValue = ref( + isDateField.value ? null : normalizeIncomingValue(props.modelValue) +) + +// DatePicker uses Date internally; parent state stays YYYY-MM-DD string +const datePickerValue = ref( + isDateField.value ? parseDateFieldValue(props.modelValue) : null +) // Watch for external changes watch( () => props.modelValue, newValue => { + if (isDateField.value) { + const parsed = parseDateFieldValue(newValue) + if (!sameCalendarDay(datePickerValue.value, parsed)) { + datePickerValue.value = parsed + } + return + } + localValue.value = normalizeIncomingValue(newValue) } ) // Watch for local changes and emit to parent watch(localValue, newValue => { + if (isDateField.value) { + return + } + emit('update:modelValue', newValue) }) +watch(datePickerValue, newDate => { + if (!isDateField.value) { + return + } + + const serialized = formatLocalDateYmd(newDate) ?? null + if (sameCalendarDay(serialized, props.modelValue)) { + return + } + + emit('update:modelValue', serialized) +}) + // Handle blur event const handleBlur = () => { emit('blur', { fieldId: props.fieldConfig.id, - value: localValue.value, + value: isDateField.value + ? (formatLocalDateYmd(datePickerValue.value) ?? null) + : localValue.value, }) } diff --git a/vueManager/src/components/ExtraFieldsManager.vue b/vueManager/src/components/ExtraFieldsManager.vue index 545ba44f5..e6e3f300d 100644 --- a/vueManager/src/components/ExtraFieldsManager.vue +++ b/vueManager/src/components/ExtraFieldsManager.vue @@ -29,6 +29,7 @@ import { parseRepeaterConfig, REPEATER_XTYPE, } from '../utils/repeaterField.js' +import { DATEFIELD_XTYPE } from '../utils/structuredExtraField.js' import KeyValueSchemaEditor from './KeyValueSchemaEditor.vue' import RepeaterSchemaEditor from './RepeaterSchemaEditor.vue' @@ -106,6 +107,7 @@ const xtypeOptions = computed(() => [ { label: _('ms3_vue_xtype_combo_vendor'), value: 'ms3-combo-vendor' }, { label: _('ms3_vue_xtype_combo_autocomplete'), value: 'ms3-combo-autocomplete' }, { label: _('ms3_vue_xtype_combo_options'), value: 'ms3-combo-options' }, + { label: _('ms3_vue_xtype_datefield'), value: DATEFIELD_XTYPE }, ]) /** @@ -116,6 +118,7 @@ const dbtypeOptions = computed(() => [ { label: _('ms3_vue_dbtype_text'), value: 'text' }, { label: _('ms3_vue_dbtype_int'), value: 'int' }, { label: _('ms3_vue_dbtype_decimal'), value: 'decimal' }, + { label: _('ms3_vue_dbtype_date'), value: 'date' }, { label: _('ms3_vue_dbtype_datetime'), value: 'datetime' }, { label: _('ms3_vue_dbtype_timestamp'), value: 'timestamp' }, { label: _('ms3_vue_dbtype_tinyint'), value: 'tinyint' }, @@ -158,18 +161,20 @@ const indexTypeOptions = computed(() => [ const isRepeaterField = computed(() => fieldForm.value.xtype === REPEATER_XTYPE) const isKeyValueField = computed(() => fieldForm.value.xtype === KEY_VALUE_XTYPE) +const XTYPE_DB_DEFAULTS = { + [REPEATER_XTYPE]: { dbtype: 'json', phptype: 'json', precision: '', null: true }, + [KEY_VALUE_XTYPE]: { dbtype: 'json', phptype: 'json', precision: '', null: true }, + [DATEFIELD_XTYPE]: { dbtype: 'date', phptype: 'datetime', precision: '', null: true }, +} + watch( () => fieldForm.value.xtype, xtype => { - if (xtype !== REPEATER_XTYPE && xtype !== KEY_VALUE_XTYPE) { - return + const defaults = XTYPE_DB_DEFAULTS[xtype] + if (defaults) { + Object.assign(fieldForm.value, defaults) } - fieldForm.value.dbtype = 'json' - fieldForm.value.phptype = 'json' - fieldForm.value.precision = '' - fieldForm.value.null = true - if (xtype === REPEATER_XTYPE && !fieldForm.value.repeater_config?.columns?.length) { fieldForm.value.repeater_config = defaultRepeaterConfig() } diff --git a/vueManager/src/utils/structuredExtraField.js b/vueManager/src/utils/structuredExtraField.js index f83c14506..301232fec 100644 --- a/vueManager/src/utils/structuredExtraField.js +++ b/vueManager/src/utils/structuredExtraField.js @@ -1,13 +1,46 @@ import { KEY_VALUE_XTYPE, parseKeyValueModelValue } from './keyValueField.js' import { parseRepeaterModelValue, REPEATER_XTYPE } from './repeaterField.js' +export const DATEFIELD_XTYPE = 'datefield' + const STRUCTURED_EXTRA_FIELD_PARSERS = { [REPEATER_XTYPE]: parseRepeaterModelValue, [KEY_VALUE_XTYPE]: parseKeyValueModelValue, } +const FULL_WIDTH_EXTRA_FIELD_XTYPES = new Set([REPEATER_XTYPE, KEY_VALUE_XTYPE]) + +/** + * Parse stored date (YYYY-MM-DD or ISO) into a local Date for DatePicker. + * + * @param {string|number|Date|null|undefined} value + * @returns {Date|null} + */ +export function parseDateFieldValue(value) { + if (value == null || value === '') { + return null + } + + if (value instanceof Date) { + return Number.isNaN(value.getTime()) ? null : value + } + + if (typeof value !== 'string') { + return null + } + + const ymd = value.match(/^(\d{4})-(\d{2})-(\d{2})/) + if (ymd) { + const local = new Date(Number(ymd[1]), Number(ymd[2]) - 1, Number(ymd[3])) + return Number.isNaN(local.getTime()) ? null : local + } + + const parsed = new Date(value) + return Number.isNaN(parsed.getTime()) ? null : parsed +} + export function isFullWidthExtraFieldXtype(xtype) { - return Object.prototype.hasOwnProperty.call(STRUCTURED_EXTRA_FIELD_PARSERS, xtype) + return FULL_WIDTH_EXTRA_FIELD_XTYPES.has(xtype) } export function parseStructuredExtraFieldValue(xtype, value) { diff --git a/vueManager/src/utils/structuredExtraField.test.js b/vueManager/src/utils/structuredExtraField.test.js new file mode 100644 index 000000000..0835d2fba --- /dev/null +++ b/vueManager/src/utils/structuredExtraField.test.js @@ -0,0 +1,42 @@ +import { describe, expect, it } from 'vitest' + +import { formatLocalDateYmd } from './formatLocalDateYmd.js' +import { KEY_VALUE_XTYPE } from './keyValueField.js' +import { REPEATER_XTYPE } from './repeaterField.js' +import { + DATEFIELD_XTYPE, + isFullWidthExtraFieldXtype, + parseDateFieldValue, + parseStructuredExtraFieldValue, +} from './structuredExtraField.js' + +describe('structuredExtraField datefield', () => { + it('does not treat datefield as full-width layout', () => { + expect(isFullWidthExtraFieldXtype(DATEFIELD_XTYPE)).toBe(false) + }) + + it('parses YYYY-MM-DD into local calendar Date', () => { + const parsed = parseDateFieldValue('2026-04-20') + expect(parsed).toBeInstanceOf(Date) + expect(parsed.getFullYear()).toBe(2026) + expect(parsed.getMonth()).toBe(3) + expect(parsed.getDate()).toBe(20) + }) + + it('serializes Date without UTC ISO shift', () => { + const localMidnight = new Date(2026, 3, 20, 0, 0, 0) + expect(formatLocalDateYmd(localMidnight)).toBe('2026-04-20') + expect(formatLocalDateYmd(localMidnight)).not.toBe(localMidnight.toISOString()) + }) + + it('leaves datefield scalar in parseStructuredExtraFieldValue', () => { + const stored = '2026-04-21' + expect(parseStructuredExtraFieldValue(DATEFIELD_XTYPE, stored)).toBe(stored) + expect(parseStructuredExtraFieldValue(DATEFIELD_XTYPE, null)).toBeNull() + }) + + it('still parses repeater and key-value structured values', () => { + expect(parseStructuredExtraFieldValue(REPEATER_XTYPE, '[]')).toEqual([]) + expect(parseStructuredExtraFieldValue(KEY_VALUE_XTYPE, '{}')).toEqual({}) + }) +}) From e482fe3c9d73fea46ecba897dfcc1051edcef56e Mon Sep 17 00:00:00 2001 From: Ibochkarev <2138260+Ibochkarev@users.noreply.github.com> Date: Sun, 6 Sep 2026 08:27:29 +0200 Subject: [PATCH 09/19] =?UTF-8?q?PR=20#624:=20=D0=9E=D0=B1=D0=BD=D0=B0?= =?UTF-8?q?=D1=80=D1=83=D0=B6=D0=B5=D0=BD=D0=B8=D0=B5=20=D1=80=D0=B0=D1=81?= =?UTF-8?q?=D1=81=D0=B8=D0=BD=D1=85=D1=80=D0=BE=D0=BD=D0=B0=20=D0=B2=D0=B5?= =?UTF-8?q?=D1=80=D1=81=D0=B8=D0=B8=20=D0=BF=D0=B0=D0=BA=D0=B5=D1=82=D0=B0?= =?UTF-8?q?=20=D0=B8=20=D1=84=D0=B0=D0=B9=D0=BB=D0=BE=D0=B2=20=D0=BD=D0=B0?= =?UTF-8?q?=20=D0=B4=D0=B8=D1=81=D0=BA=D0=B5?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit https://github.com/modx-pro/MiniShop3/pull/624 --- _build/elements/settings.php | 5 + _build/resolvers/resolver_09_version.php | 67 +++++++ .../minishop3/config/routes/manager.php | 2 +- .../minishop3/config/routes/web.php | 2 +- .../minishop3/elements/plugins/minishop3.php | 96 +++++++++- .../minishop3/lexicon/en/default.inc.php | 2 + .../minishop3/lexicon/en/setting.inc.php | 2 + .../minishop3/lexicon/ru/default.inc.php | 2 + .../minishop3/lexicon/ru/setting.inc.php | 2 + .../Unit/Utils/VersionSyncContractTest.php | 176 ++++++++++++++++++ 10 files changed, 349 insertions(+), 7 deletions(-) create mode 100644 _build/resolvers/resolver_09_version.php create mode 100644 core/components/minishop3/tests/Unit/Utils/VersionSyncContractTest.php diff --git a/_build/elements/settings.php b/_build/elements/settings.php index fa5958b21..83d563e2f 100644 --- a/_build/elements/settings.php +++ b/_build/elements/settings.php @@ -19,6 +19,11 @@ 'xtype' => 'textfield', 'area' => 'ms3_main', ], + 'ms3_version' => [ + 'value' => '', + 'xtype' => 'textfield', + 'area' => 'ms3_main', + ], 'ms3_category_show_nested_products' => [ 'value' => true, diff --git a/_build/resolvers/resolver_09_version.php b/_build/resolvers/resolver_09_version.php new file mode 100644 index 000000000..c832f850e --- /dev/null +++ b/_build/resolvers/resolver_09_version.php @@ -0,0 +1,67 @@ +xpdo || !($transport instanceof xPDOTransport)) { + return true; +} + +$modx = $transport->xpdo; + +if (!in_array($options[xPDOTransport::PACKAGE_ACTION], [ + xPDOTransport::ACTION_INSTALL, + xPDOTransport::ACTION_UPGRADE, +], true)) { + return true; +} + +$packageVersion = ''; +if (!empty($transport->signature)) { + [, $packageVersion] = xPDOTransport::parseSignature((string)$transport->signature); + $packageVersion = (string)$packageVersion; +} + +if ($packageVersion === '') { + $modx->log(modX::LOG_LEVEL_WARN, '[MiniShop3] Could not determine package version from transport signature.'); + return true; +} + +/** @var modSystemSetting|null $setting */ +$setting = $modx->getObject(modSystemSetting::class, ['key' => 'ms3_version']); +if (!$setting) { + $setting = $modx->newObject(modSystemSetting::class); + $setting->fromArray([ + 'key' => 'ms3_version', + 'namespace' => 'minishop3', + 'area' => 'ms3_main', + 'xtype' => 'textfield', + 'value' => '', + ], '', true, true); +} + +$setting->set('value', $packageVersion); +if (!$setting->save()) { + $modx->log( + modX::LOG_LEVEL_ERROR, + '[MiniShop3] Failed to save ms3_version system setting to ' . $packageVersion + ); + + return true; +} + +$modx->log(modX::LOG_LEVEL_INFO, '[MiniShop3] Set ms3_version system setting to ' . $packageVersion); + +return true; diff --git a/core/components/minishop3/config/routes/manager.php b/core/components/minishop3/config/routes/manager.php index 28066761a..a6d76f6a5 100644 --- a/core/components/minishop3/config/routes/manager.php +++ b/core/components/minishop3/config/routes/manager.php @@ -34,7 +34,7 @@ $router->get('/health', function () use ($modx) { return Response::success([ 'status' => 'ok', - 'version' => $modx->getOption('ms3_version', null, '1.0.0'), + 'version' => $modx->getOption('ms3_version', null, '1.0.0', true), 'timestamp' => time(), 'api' => 'manager' ]); diff --git a/core/components/minishop3/config/routes/web.php b/core/components/minishop3/config/routes/web.php index a2e998765..1ab357bc6 100644 --- a/core/components/minishop3/config/routes/web.php +++ b/core/components/minishop3/config/routes/web.php @@ -337,7 +337,7 @@ $router->get('/health', function () use ($modx) { return Response::success([ 'status' => 'ok', - 'version' => $modx->getOption('ms3_version', null, '1.0.0'), + 'version' => $modx->getOption('ms3_version', null, '1.0.0', true), 'timestamp' => time(), 'api' => 'web' ]); diff --git a/core/components/minishop3/elements/plugins/minishop3.php b/core/components/minishop3/elements/plugins/minishop3.php index c12a939fa..92530d16a 100644 --- a/core/components/minishop3/elements/plugins/minishop3.php +++ b/core/components/minishop3/elements/plugins/minishop3.php @@ -34,17 +34,103 @@ break; case 'OnManagerPageBeforeRender': - if (!$modx->services->has('ms3')) { - $modx->log(\MODX\Revolution\modX::LOG_LEVEL_ERROR, '[MiniShop3] Service not registered'); + // Editors cannot fix a copy failure; limit noise to mgr sessions (#622 review). + if (!$modx->user || !$modx->user->hasSessionContext('mgr')) { break; } - /** @var \MiniShop3\MiniShop3 $ms3 */ - $ms3 = $modx->services->get('ms3'); + + // Version check runs before the ms3 service guard so a total copy failure + // (no core/components/minishop3 on disk) still surfaces a banner (#622). + $packageVersion = (string)$modx->getOption('ms3_version', null, ''); + $diskVersion = ''; + /** @var \MiniShop3\MiniShop3|null $ms3 */ + $ms3 = null; + if ($modx->services->has('ms3')) { + $ms3 = $modx->services->get('ms3'); + $diskVersion = (string)$ms3->version; + } + + // Warn when disk is missing or lags the installed package — newer disk (git/rsync) is OK. + $versionMismatch = $packageVersion !== '' + && ($diskVersion === '' || version_compare($diskVersion, $packageVersion, '<')); + + if ($versionMismatch) { + if (isset($modx->controller)) { + $modx->controller->addLexiconTopic('minishop3:default'); + } else { + $modx->lexicon->load('minishop3:default'); + } + + $filesMissing = $diskVersion === ''; + $lexiconKey = $filesMissing ? 'ms3_version_files_missing' : 'ms3_version_mismatch_warning'; + $message = $modx->lexicon($lexiconKey, $filesMissing + ? ['package' => $packageVersion] + : ['disk' => $diskVersion, 'package' => $packageVersion]); + $message = is_string($message) ? $message : ''; + if ($message === $lexiconKey || $message === '') { + $message = $filesMissing + ? 'MiniShop3 component files were not found on disk' + . ' (installed package ' . $packageVersion . ').' + . ' Check write permissions for core/components/minishop3/' + . ' and assets/components/minishop3/, then reinstall the package.' + : 'MiniShop3 version mismatch: files on disk (' . $diskVersion + . ') are older than the installed package (' . $packageVersion + . '). The database was updated but component files may not have been copied.' + . ' Check write permissions for core/components/minishop3/' + . ' and assets/components/minishop3/.'; + } + + $messageHtml = htmlspecialchars($message, ENT_QUOTES, 'UTF-8'); + $versionKey = md5($diskVersion . '|' . $packageVersion); + $modx->regClientHTMLBlock( + '
' + . 'MiniShop3: ' . $messageHtml + . '' + . '
' + . '' + . '' + ); + + // Once per mgr session — avoid filling error.log on every page render (#622). + $logKey = 'ms3_version_mismatch_logged_' . $versionKey; + if (empty($_SESSION[$logKey])) { + $_SESSION[$logKey] = true; + $modx->log( + modX::LOG_LEVEL_WARN, + '[MiniShop3] Version mismatch: disk=' . ($diskVersion !== '' ? $diskVersion : '(missing)') + . ', package=' . $packageVersion + ); + } + } + + if ($ms3 === null) { + if (empty($_SESSION['ms3_service_not_registered_logged'])) { + $_SESSION['ms3_service_not_registered_logged'] = true; + $modx->log(modX::LOG_LEVEL_WARN, '[MiniShop3] Service not registered'); + } + break; + } + $modx->controller->addLexiconTopic('minishop3:default'); $modx->regClientStartupScript($ms3->config['jsUrl'] . 'mgr/misc/ms3.manager.js'); $syncEnabled = (bool)$modx->getOption('ms3_customer_sync_enabled', null, false); - if ($syncEnabled && $modx->user && $modx->user->hasSessionContext('mgr')) { + if ($syncEnabled) { $modx->lexicon->load('minishop3:customer'); } break; diff --git a/core/components/minishop3/lexicon/en/default.inc.php b/core/components/minishop3/lexicon/en/default.inc.php index d8cea3577..cc1889be8 100644 --- a/core/components/minishop3/lexicon/en/default.inc.php +++ b/core/components/minishop3/lexicon/en/default.inc.php @@ -28,6 +28,8 @@ $_lang['ms3_settings_desc'] = 'Order statuses, payment and delivery parameters'; $_lang['ms3_system_settings'] = 'System settings'; $_lang['ms3_system_settings_desc'] = 'MiniShop3 system settings'; +$_lang['ms3_version_mismatch_warning'] = 'MiniShop3 version mismatch: files on disk ([[+disk]]) are older than the installed package ([[+package]]). The database was updated but component files may not have been copied. Check write permissions for core/components/minishop3/ and assets/components/minishop3/.'; +$_lang['ms3_version_files_missing'] = 'MiniShop3 component files were not found on disk (installed package [[+package]]). Check write permissions for core/components/minishop3/ and assets/components/minishop3/, then reinstall the package.'; $_lang['ms3_utilities'] = 'Utilities'; $_lang['ms3_utilities_desc'] = 'Developer tools'; $_lang['ms3_grid_fields_config_desc'] = 'Table Fields'; diff --git a/core/components/minishop3/lexicon/en/setting.inc.php b/core/components/minishop3/lexicon/en/setting.inc.php index 892dbc979..69c9a0586 100644 --- a/core/components/minishop3/lexicon/en/setting.inc.php +++ b/core/components/minishop3/lexicon/en/setting.inc.php @@ -24,6 +24,8 @@ $_lang['setting_ms3_chunks_categories'] = 'Categories for chunks list'; $_lang['setting_ms3_chunks_categories_desc'] = 'Comma-separated list of category IDs for chunks list.'; +$_lang['setting_ms3_version'] = 'Installed package version'; +$_lang['setting_ms3_version_desc'] = 'Version of the last successfully installed MiniShop3 transport package. Used for health checks and detecting file copy failures.'; $_lang['setting_ms3_tmp_storage'] = 'Cart and temporary order fields storage'; $_lang['setting_ms3_tmp_storage_desc'] = " To store cart and temporary order fields in session specify session
diff --git a/core/components/minishop3/lexicon/ru/default.inc.php b/core/components/minishop3/lexicon/ru/default.inc.php index 04be9deb1..eb7435f0f 100644 --- a/core/components/minishop3/lexicon/ru/default.inc.php +++ b/core/components/minishop3/lexicon/ru/default.inc.php @@ -28,6 +28,8 @@ $_lang['ms3_settings_desc'] = 'Статусы заказов, параметры оплаты и доставки'; $_lang['ms3_system_settings'] = 'Системные настройки'; $_lang['ms3_system_settings_desc'] = 'Системные настройки MiniShop3'; +$_lang['ms3_version_mismatch_warning'] = 'Несовпадение версий MiniShop3: файлы на диске ([[+disk]]) старше установленного пакета ([[+package]]). База данных обновлена, но файлы компонента могли не скопироваться. Проверьте права на запись для core/components/minishop3/ и assets/components/minishop3/.'; +$_lang['ms3_version_files_missing'] = 'Файлы компонента MiniShop3 не найдены на диске (установленный пакет [[+package]]). Проверьте права на запись для core/components/minishop3/ и assets/components/minishop3/, затем переустановите пакет.'; $_lang['ms3_utilities'] = 'Утилиты'; $_lang['ms3_utilities_desc'] = 'Инструменты разработчика'; $_lang['ms3_grid_fields_config_desc'] = 'Поля таблиц'; diff --git a/core/components/minishop3/lexicon/ru/setting.inc.php b/core/components/minishop3/lexicon/ru/setting.inc.php index a1aea93f5..410402fa9 100644 --- a/core/components/minishop3/lexicon/ru/setting.inc.php +++ b/core/components/minishop3/lexicon/ru/setting.inc.php @@ -24,6 +24,8 @@ $_lang['setting_ms3_chunks_categories'] = 'Категории для списка чанков'; $_lang['setting_ms3_chunks_categories_desc'] = 'Список ID категорий через запятую для списка чанков.'; +$_lang['setting_ms3_version'] = 'Версия установленного пакета'; +$_lang['setting_ms3_version_desc'] = 'Версия последнего успешно установленного транспортного пакета MiniShop3. Используется для health-check и обнаружения сбоев копирования файлов.'; $_lang['setting_ms3_tmp_storage'] = 'Хранилище корзины и временных полей заказа'; $_lang['setting_ms3_tmp_storage_desc'] = " Для хранения корзины и временных полей заказа в сессии укажите session
diff --git a/core/components/minishop3/tests/Unit/Utils/VersionSyncContractTest.php b/core/components/minishop3/tests/Unit/Utils/VersionSyncContractTest.php new file mode 100644 index 000000000..881216cbe --- /dev/null +++ b/core/components/minishop3/tests/Unit/Utils/VersionSyncContractTest.php @@ -0,0 +1,176 @@ + 0 && is_numeric(substr($part, 0, $dotPos))) { + $version = $part; + while (($part = next($exploded)) !== false) { + $version .= '-' . $part; + } + break; + } + $name .= '-' . $part; + $part = next($exploded); + } + + return $version; + } + + /** + * Mirrors the plugin mismatch predicate: warn only when disk is missing or behind. + */ + private static function isMismatch(string $diskVersion, string $packageVersion): bool + { + return $packageVersion !== '' + && ($diskVersion === '' || version_compare($diskVersion, $packageVersion, '<')); + } + + #[DataProvider('extractFromTransportSignatureCases')] + public function testExtractFromTransportSignature(string $signature, string $expected): void + { + self::assertSame($expected, self::extractFromTransportSignature($signature)); + } + + /** + * @return iterable + */ + public static function extractFromTransportSignatureCases(): iterable + { + yield 'standard beta signature' => ['minishop3-1.13.0-beta1', '1.13.0-beta1']; + yield 'release without suffix' => ['minishop3-1.0.0', '1.0.0']; + yield 'hyphenated package name' => ['my-extra-1.2.3-pl', '1.2.3-pl']; + yield 'empty signature' => ['', '']; + yield 'no dash separator' => ['minishop3', '']; + yield 'unknown prefix only' => ['unknown', '']; + } + + #[DataProvider('isMismatchCases')] + public function testIsMismatch(string $diskVersion, string $packageVersion, bool $expected): void + { + self::assertSame($expected, self::isMismatch($diskVersion, $packageVersion)); + } + + /** + * @return iterable + */ + public static function isMismatchCases(): iterable + { + yield 'empty package version' => ['1.13.0-beta1', '', false]; + yield 'equal versions' => ['1.13.0-beta1', '1.13.0-beta1', false]; + yield 'disk older than package' => ['1.12.0', '1.13.0-beta1', true]; + yield 'disk empty package set' => ['', '1.13.0-beta1', true]; + yield 'disk newer than package (git/rsync)' => ['1.14.0-dev', '1.13.0-beta1', false]; + } + + public function testPluginAndResolverStayDiskIndependent(): void + { + $ms3Root = dirname(__DIR__, 3); + $repoRoot = dirname($ms3Root, 3); + $plugin = file_get_contents($ms3Root . '/elements/plugins/minishop3.php'); + $resolver = file_get_contents($repoRoot . '/_build/resolvers/resolver_09_version.php'); + + self::assertIsString($plugin); + self::assertIsString($resolver); + + self::assertStringNotContainsString( + 'use MiniShop3\\Utils\\', + $plugin, + 'Plugin plugincode is DB-static and must not autoload src helpers (#622).' + ); + self::assertDoesNotMatchRegularExpression( + '/^\s*use\s+MiniShop3\\\\Utils\\\\/m', + $resolver, + 'Resolver must run from transport even when component src was not copied (#622).' + ); + + self::assertStringContainsString("getOption('ms3_version'", $plugin); + self::assertStringContainsString('ms3-version-mismatch-banner', $plugin); + self::assertMatchesRegularExpression( + "/version_compare\\(\\\$diskVersion, \\\$packageVersion, '<'\\)/", + $plugin, + 'Plugin must warn only when disk lags package (not strict inequality).' + ); + self::assertStringContainsString('regClientHTMLBlock', $plugin); + self::assertStringContainsString("'key' => 'ms3_version'", $resolver); + self::assertStringContainsString('parseSignature', $resolver); + self::assertStringNotContainsString("explode('-'", $resolver); + } + + public function testHealthRoutesReferenceMs3VersionWithSkipEmpty(): void + { + $ms3Root = dirname(__DIR__, 3); + + foreach (['config/routes/manager.php', 'config/routes/web.php'] as $routeFile) { + $contents = file_get_contents($ms3Root . '/' . $routeFile); + self::assertIsString($contents); + self::assertStringContainsString( + "getOption('ms3_version', null, '1.0.0', true)", + $contents + ); + } + } + + public function testDiskVersionMatchesBuildConfig(): void + { + $ms3Root = dirname(__DIR__, 3); + $repoRoot = dirname($ms3Root, 3); + + $miniShop3Php = file_get_contents($ms3Root . '/src/MiniShop3.php'); + $buildConfig = file_get_contents($repoRoot . '/_build/config.inc.php'); + self::assertIsString($miniShop3Php); + self::assertIsString($buildConfig); + + self::assertSame( + 1, + preg_match("/public\\s+\\\$version\\s*=\\s*'([^']+)'/", $miniShop3Php, $diskMatch), + 'MiniShop3::$version must be a public string property.' + ); + self::assertSame( + 1, + preg_match("/'version'\\s*=>\\s*'([^']+)'/", $buildConfig, $versionMatch), + '_build/config.inc.php must define version.' + ); + self::assertSame( + 1, + preg_match("/'release'\\s*=>\\s*'([^']+)'/", $buildConfig, $releaseMatch), + '_build/config.inc.php must define release.' + ); + + self::assertSame( + $versionMatch[1] . '-' . $releaseMatch[1], + $diskMatch[1], + 'MiniShop3::$version must equal config version-release or the mgr banner will false-positive.' + ); + } +} From 78172c653440902b6a3d98e17e0c52020ca99b20 Mon Sep 17 00:00:00 2001 From: Ibochkarev <2138260+Ibochkarev@users.noreply.github.com> Date: Sun, 6 Sep 2026 08:27:29 +0200 Subject: [PATCH 10/19] =?UTF-8?q?PR=20#627:=20=D0=98=D0=BD=D0=B4=D0=B8?= =?UTF-8?q?=D0=B2=D0=B8=D0=B4=D1=83=D0=B0=D0=BB=D1=8C=D0=BD=D1=8B=D0=B9=20?= =?UTF-8?q?menuindex=20=D0=B4=D0=BB=D1=8F=20=D1=82=D0=BE=D0=B2=D0=B0=D1=80?= =?UTF-8?q?=D0=BE=D0=B2=20=D0=B2=20=D0=B4=D0=BE=D0=BF=D0=BE=D0=BB=D0=BD?= =?UTF-8?q?=D0=B8=D1=82=D0=B5=D0=BB=D1=8C=D0=BD=D1=8B=D1=85=20=D0=BA=D0=B0?= =?UTF-8?q?=D1=82=D0=B5=D0=B3=D0=BE=D1=80=D0=B8=D1=8F=D1=85?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit https://github.com/modx-pro/MiniShop3/pull/627 --- .../elements/snippets/ms3_products.php | 27 ++ ...00_add_menuindex_to_product_categories.php | 76 ++++++ .../schema/minishop3.mysql.schema.xml | 6 + .../Manager/CategoryProductsController.php | 5 +- .../minishop3/src/Model/msCategoryMember.php | 1 + .../src/Model/mysql/msCategoryMember.php | 32 +++ .../src/Processors/Product/Category.php | 10 +- .../CategoryProductMenuindexService.php | 232 ++++++++++++++++++ .../Category/CategoryProductScopeService.php | 13 +- .../Category/CategoryProductsListService.php | 52 ++-- .../Product/ProductCatalogService.php | 67 ++++- .../ProductCategoryMembershipWriter.php | 55 ++++- .../CategoryProductMenuindexServiceTest.php | 86 +++++++ .../tests/CategoryProductScopeServiceTest.php | 15 ++ .../CategoryProductsControllerScopeTest.php | 34 +++ .../tests/ProductCatalogServiceTest.php | 3 + .../ProductCategoryMembershipWriterTest.php | 110 +++++++++ .../stubs/CategoryProductScopeModxStub.php | 31 ++- .../ProductCategoryMembershipModxStub.php | 227 +++++++++++++++++ .../tests/stubs/StubMsCategoryMember.php | 51 ++++ 20 files changed, 1080 insertions(+), 53 deletions(-) create mode 100644 core/components/minishop3/migrations/20260822120000_add_menuindex_to_product_categories.php create mode 100644 core/components/minishop3/src/Services/Category/CategoryProductMenuindexService.php create mode 100644 core/components/minishop3/tests/CategoryProductMenuindexServiceTest.php create mode 100644 core/components/minishop3/tests/ProductCategoryMembershipWriterTest.php create mode 100644 core/components/minishop3/tests/stubs/ProductCategoryMembershipModxStub.php create mode 100644 core/components/minishop3/tests/stubs/StubMsCategoryMember.php diff --git a/core/components/minishop3/elements/snippets/ms3_products.php b/core/components/minishop3/elements/snippets/ms3_products.php index 9ce99f2c1..82fc219b8 100644 --- a/core/components/minishop3/elements/snippets/ms3_products.php +++ b/core/components/minishop3/elements/snippets/ms3_products.php @@ -6,7 +6,9 @@ use MiniShop3\Model\msProductFile; use MiniShop3\Model\msProductLink; use MiniShop3\Model\msProductOption; +use MiniShop3\Model\msCategoryMember; use MiniShop3\Model\msVendor; +use MiniShop3\Services\Category\CategoryProductMenuindexService; use MiniShop3\Services\Category\CategoryProductScopeService; use MiniShop3\Utils\EventGate; use MiniShop3\Utils\ProductThumbnailJoin; @@ -157,6 +159,31 @@ } } +// Per-category menuindex for additional categories when sorting by menuindex (#625). +$_ms3MenuindexCategoryIds = []; +if (isset($_ms3CategoryIds) && $_ms3CategoryIds !== []) { + $_ms3MenuindexCategoryIds = $_ms3CategoryIds; +} else { + $_ms3SingleParent = (int)($scriptProperties['parent'] ?? $scriptProperties['category'] ?? 0); + if ($_ms3SingleParent > 0) { + $_ms3MenuindexCategoryIds = [$_ms3SingleParent]; + } +} +$_ms3SortBy = (string)($scriptProperties['sortby'] ?? ''); +if ($_ms3MenuindexCategoryIds !== [] && preg_match('/\bmenuindex\b/i', $_ms3SortBy)) { + $memberAlias = CategoryProductMenuindexService::MEMBER_JOIN_ALIAS; + $leftJoin[$memberAlias] = [ + 'class' => msCategoryMember::class, + 'on' => CategoryProductMenuindexService::memberJoinOnCategories($_ms3MenuindexCategoryIds, $memberAlias), + ]; + $effectiveSql = CategoryProductMenuindexService::effectiveMenuindexSqlForCategories($_ms3MenuindexCategoryIds); + if (preg_match('/\bmsProduct\.menuindex\b/i', $_ms3SortBy)) { + $scriptProperties['sortby'] = preg_replace('/\bmsProduct\.menuindex\b/i', $effectiveSql, $_ms3SortBy); + } elseif (preg_match('/\bmenuindex\b/i', $_ms3SortBy) && !str_contains($_ms3SortBy, 'CASE WHEN')) { + $scriptProperties['sortby'] = preg_replace('/\bmenuindex\b/i', $effectiveSql, $_ms3SortBy, 1); + } +} + // Add filters by options $joinedOptions = []; if (!empty($scriptProperties['optionFilters'])) { diff --git a/core/components/minishop3/migrations/20260822120000_add_menuindex_to_product_categories.php b/core/components/minishop3/migrations/20260822120000_add_menuindex_to_product_categories.php new file mode 100644 index 000000000..1694f3323 --- /dev/null +++ b/core/components/minishop3/migrations/20260822120000_add_menuindex_to_product_categories.php @@ -0,0 +1,76 @@ +table(self::TABLE); + $columnAdded = false; + + if (!$table->hasColumn('menuindex')) { + $table->addColumn('menuindex', 'integer', [ + 'signed' => false, + 'null' => false, + 'default' => 0, + 'after' => 'category_id', + ]); + $table->update(); + $columnAdded = true; + } + + if ($columnAdded) { + $this->backfillMenuindexFromProducts(); + } + + $table = $this->table(self::TABLE); + if (!$table->hasIndexByName(self::INDEX_CATEGORY_MENUINDEX)) { + $table->addIndex(['category_id', 'menuindex'], [ + 'name' => self::INDEX_CATEGORY_MENUINDEX, + ]); + $table->update(); + } + } + + public function down(): void + { + $table = $this->table(self::TABLE); + + if ($table->hasIndexByName(self::INDEX_CATEGORY_MENUINDEX)) { + $table->removeIndexByName(self::INDEX_CATEGORY_MENUINDEX); + } + + if ($table->hasColumn('menuindex')) { + $table->removeColumn('menuindex'); + } + + $table->update(); + } + + private function backfillMenuindexFromProducts(): void + { + if (!$this->hasTable(self::TABLE)) { + return; + } + + $prefix = (string) ($this->getAdapter()->getOption('table_prefix') ?? ''); + $members = '`' . $prefix . self::TABLE . '`'; + $resources = '`' . $prefix . 'site_content`'; + + $this->execute( + "UPDATE {$members} AS m " + . "INNER JOIN {$resources} AS sc ON sc.id = m.product_id " + . 'SET m.menuindex = sc.menuindex' + ); + } +} diff --git a/core/components/minishop3/schema/minishop3.mysql.schema.xml b/core/components/minishop3/schema/minishop3.mysql.schema.xml index 467788366..db1b744bc 100644 --- a/core/components/minishop3/schema/minishop3.mysql.schema.xml +++ b/core/components/minishop3/schema/minishop3.mysql.schema.xml @@ -106,11 +106,17 @@ index="pk"/> + + + + + scopeService(); + $menuindexService = new CategoryProductMenuindexService($this->modx); foreach ($items as $item) { $productId = (int) ($item['id'] ?? 0); @@ -186,8 +188,7 @@ public function sort(array $params = []): array continue; } - $product->set('menuindex', $menuindex); - if ($product->save()) { + if ($menuindexService->setMenuindexInCategory($productId, $categoryId, $menuindex)) { $updated++; } } diff --git a/core/components/minishop3/src/Model/msCategoryMember.php b/core/components/minishop3/src/Model/msCategoryMember.php index 8efad1748..8364f5ca9 100644 --- a/core/components/minishop3/src/Model/msCategoryMember.php +++ b/core/components/minishop3/src/Model/msCategoryMember.php @@ -9,6 +9,7 @@ * * @property integer $product_id * @property integer $category_id + * @property integer $menuindex * * @package MiniShop3\Model */ diff --git a/core/components/minishop3/src/Model/mysql/msCategoryMember.php b/core/components/minishop3/src/Model/mysql/msCategoryMember.php index 34ad02251..9de04659a 100644 --- a/core/components/minishop3/src/Model/mysql/msCategoryMember.php +++ b/core/components/minishop3/src/Model/mysql/msCategoryMember.php @@ -17,6 +17,7 @@ class msCategoryMember extends \MiniShop3\Model\msCategoryMember [ 'product_id' => null, 'category_id' => null, + 'menuindex' => 0, ], 'fieldMeta' => [ @@ -38,6 +39,15 @@ class msCategoryMember extends \MiniShop3\Model\msCategoryMember 'null' => false, 'index' => 'pk', ], + 'menuindex' => + [ + 'dbtype' => 'int', + 'precision' => '10', + 'attributes' => 'unsigned', + 'phptype' => 'integer', + 'null' => false, + 'default' => 0, + ], ], 'indexes' => [ @@ -63,6 +73,28 @@ class msCategoryMember extends \MiniShop3\Model\msCategoryMember ], ], ], + 'category_menuindex' => + [ + 'alias' => 'category_menuindex', + 'primary' => false, + 'unique' => false, + 'type' => 'BTREE', + 'columns' => + [ + 'category_id' => + [ + 'length' => '', + 'collation' => 'A', + 'null' => false, + ], + 'menuindex' => + [ + 'length' => '', + 'collation' => 'A', + 'null' => false, + ], + ], + ], ], 'aggregates' => [ diff --git a/core/components/minishop3/src/Processors/Product/Category.php b/core/components/minishop3/src/Processors/Product/Category.php index 78e011ec7..ae6cc4589 100644 --- a/core/components/minishop3/src/Processors/Product/Category.php +++ b/core/components/minishop3/src/Processors/Product/Category.php @@ -3,6 +3,7 @@ namespace MiniShop3\Processors\Product; use MiniShop3\Model\msCategoryMember; +use MiniShop3\Services\Category\CategoryProductMenuindexService; use MODX\Revolution\Processors\Model\CreateProcessor; class Category extends CreateProcessor @@ -22,13 +23,10 @@ public function process() /** @var msCategoryMember $res */ $res = $this->modx->getObject(msCategoryMember::class, ['category_id' => $cid, 'product_id' => $pid]); if (!$res) { - $res = $this->modx->newObject(msCategoryMember::class); - $res->set('product_id', $pid); - $res->set('category_id', $cid); - $res->save(); + $menuindexService = new CategoryProductMenuindexService($this->modx); + $menuindexService->ensureMember((int) $pid, (int) $cid); } else { - $table = $this->modx->getTableName(msCategoryMember::class); - $this->modx->exec("DELETE FROM {$table} WHERE `product_id` = {$pid} AND `category_id` = {$cid};"); + $res->remove(); } } diff --git a/core/components/minishop3/src/Services/Category/CategoryProductMenuindexService.php b/core/components/minishop3/src/Services/Category/CategoryProductMenuindexService.php new file mode 100644 index 000000000..6214db92b --- /dev/null +++ b/core/components/minishop3/src/Services/Category/CategoryProductMenuindexService.php @@ -0,0 +1,232 @@ + $categoryIds + */ + public static function isNativeInCategories(int $productParent, array $categoryIds): bool + { + return in_array($productParent, $categoryIds, true); + } + + /** + * Effective sort/select expression for a single category grid context. + */ + public static function effectiveMenuindexSql( + int $categoryId, + string $productAlias = 'msProduct', + string $memberAlias = self::MEMBER_JOIN_ALIAS, + ): string { + return self::effectiveMenuindexCase( + "{$productAlias}.parent = " . max(0, $categoryId), + "{$memberAlias}.menuindex", + $productAlias, + ); + } + + /** + * Effective sort/select when multiple category IDs define the catalog scope. + * + * @param list $categoryIds + */ + public static function effectiveMenuindexSqlForCategories( + array $categoryIds, + string $productAlias = 'msProduct', + string $memberAlias = self::MEMBER_JOIN_ALIAS, + ): string { + $categoryIds = self::normalizeCategoryIds($categoryIds); + if ($categoryIds === []) { + return "{$productAlias}.menuindex"; + } + + if (count($categoryIds) === 1) { + return self::effectiveMenuindexSql($categoryIds[0], $productAlias, $memberAlias); + } + + return self::effectiveMenuindexCase( + "{$productAlias}.parent IN (" . implode(',', $categoryIds) . ')', + "MIN({$memberAlias}.menuindex)", + $productAlias, + ); + } + + public static function memberJoinOn(int $categoryId, string $memberAlias = self::MEMBER_JOIN_ALIAS): string + { + $categoryId = max(0, $categoryId); + + return "`{$memberAlias}`.product_id = msProduct.id AND `{$memberAlias}`.category_id = {$categoryId}"; + } + + /** + * @param list $categoryIds + */ + public static function memberJoinOnCategories(array $categoryIds, string $memberAlias = self::MEMBER_JOIN_ALIAS): string + { + $categoryIds = self::normalizeCategoryIds($categoryIds); + if ($categoryIds === []) { + return '1=0'; + } + + if (count($categoryIds) === 1) { + return self::memberJoinOn($categoryIds[0], $memberAlias); + } + + return "`{$memberAlias}`.product_id = msProduct.id AND `{$memberAlias}`.category_id IN (" + . implode(',', $categoryIds) + . ')'; + } + + /** + * LEFT JOIN msCategoryMember for catalog/list scope and return effective menuindex SQL. + * + * @param list $categoryIds + */ + public static function applyMemberJoin(xPDOQuery $query, array $categoryIds): string + { + $memberAlias = self::MEMBER_JOIN_ALIAS; + $query->leftJoin( + msCategoryMember::class, + $memberAlias, + self::memberJoinOnCategories($categoryIds, $memberAlias) + ); + + return self::effectiveMenuindexSqlForCategories($categoryIds); + } + + /** + * @param list $categoryIds + * @return list + */ + private static function normalizeCategoryIds(array $categoryIds): array + { + return array_values(array_unique(array_filter(array_map('intval', $categoryIds)))); + } + + private static function effectiveMenuindexCase( + string $nativeParentCondition, + string $memberMenuindexExpression, + string $productAlias, + ): string { + return 'CASE WHEN ' + . "{$nativeParentCondition} " + . "THEN {$productAlias}.menuindex " + . "ELSE COALESCE({$memberMenuindexExpression}, {$productAlias}.menuindex) " + . 'END'; + } + + public function getNextMemberMenuindex(int $categoryId): int + { + if ($categoryId <= 0) { + return 0; + } + + $maxNative = $this->fetchMaxMenuindex( + msProduct::class, + ['parent' => $categoryId], + ); + $maxMember = $this->fetchMaxMenuindex( + msCategoryMember::class, + ['category_id' => $categoryId], + ); + + return max($maxNative, $maxMember) + 1; + } + + public function ensureMember(int $productId, int $categoryId): bool + { + if ($productId <= 0 || $categoryId <= 0) { + return false; + } + + $existing = $this->modx->getObject(msCategoryMember::class, [ + 'category_id' => $categoryId, + 'product_id' => $productId, + ]); + if ($existing) { + return true; + } + + /** @var msCategoryMember $member */ + $member = $this->modx->newObject(msCategoryMember::class); + $member->set('product_id', $productId); + $member->set('category_id', $categoryId); + $member->set('menuindex', $this->getNextMemberMenuindex($categoryId)); + + return (bool) $member->save(); + } + + /** + * @param class-string $classKey + * @param array $criteria + */ + private function fetchMaxMenuindex(string $classKey, array $criteria): int + { + $query = $this->modx->newQuery($classKey); + $query->where($criteria); + $query->select('MAX(menuindex)'); + + if (!$query->prepare() || !$query->stmt->execute()) { + return -1; + } + + $max = $query->stmt->fetchColumn(); + + return $max !== false ? (int) $max : -1; + } + + public function setMenuindexInCategory(int $productId, int $categoryId, int $menuindex): bool + { + if ($productId <= 0 || $categoryId <= 0) { + return false; + } + + /** @var msProduct|null $product */ + $product = $this->modx->getObject(msProduct::class, $productId); + if (!$product) { + return false; + } + + if (self::isNativeInCategory((int) $product->get('parent'), $categoryId)) { + $product->set('menuindex', $menuindex); + + return (bool) $product->save(); + } + + /** @var msCategoryMember|null $member */ + $member = $this->modx->getObject(msCategoryMember::class, [ + 'category_id' => $categoryId, + 'product_id' => $productId, + ]); + if (!$member) { + return false; + } + + $member->set('menuindex', $menuindex); + + return (bool) $member->save(); + } +} diff --git a/core/components/minishop3/src/Services/Category/CategoryProductScopeService.php b/core/components/minishop3/src/Services/Category/CategoryProductScopeService.php index 2389034dc..6c9d4b37d 100644 --- a/core/components/minishop3/src/Services/Category/CategoryProductScopeService.php +++ b/core/components/minishop3/src/Services/Category/CategoryProductScopeService.php @@ -232,20 +232,11 @@ public static function buildProductCategoryScopeWhere(array $categoryIds, array } /** - * menuindex reorder applies only to direct children (not additional-category-only links). + * Whether drag-drop menuindex reorder is allowed in this category grid (#625). */ public function canReorderInCategory(int $productId, int $categoryId): bool { - if ($productId <= 0 || $categoryId <= 0) { - return false; - } - - $product = $this->modx->getObject(msProduct::class, $productId); - if (!$product) { - return false; - } - - return (int) $product->get('parent') === $categoryId; + return $this->findInCategory($categoryId, $productId, false) !== null; } /** diff --git a/core/components/minishop3/src/Services/Category/CategoryProductsListService.php b/core/components/minishop3/src/Services/Category/CategoryProductsListService.php index 17bd05e13..8b9f34ac8 100644 --- a/core/components/minishop3/src/Services/Category/CategoryProductsListService.php +++ b/core/components/minishop3/src/Services/Category/CategoryProductsListService.php @@ -5,6 +5,7 @@ namespace MiniShop3\Services\Category; use MiniShop3\Model\msCategory; +use MiniShop3\Model\msCategoryMember; use MiniShop3\Model\msProduct; use MiniShop3\Model\msProductData; use MiniShop3\Model\msProductOption; @@ -48,15 +49,20 @@ public function getPage( $optionSpecs = GridOptionColumnResolver::resolve($gridFields); $relationSpecs = GridRelationColumnResolver::resolve($this->modx, $gridFields); - $c = $this->buildProductListQuery($categoryId, $params, $nested, $optionSpecs, $relationSpecs); + $scopeCategoryIds = $nested + ? $this->treeService()->productParentIds($categoryId, true) + : [$categoryId]; - $countQuery = $this->buildProductListQuery($categoryId, $params, $nested, $optionSpecs, $relationSpecs); + $c = $this->buildProductListQuery($scopeCategoryIds, $params, $optionSpecs, $relationSpecs); + + $countQuery = $this->buildProductListQuery($scopeCategoryIds, $params, $optionSpecs, $relationSpecs); $countQuery->select('COUNT(DISTINCT msProduct.id)'); $countQuery->prepare(); $countQuery->stmt->execute(); $total = (int) $countQuery->stmt->fetchColumn(); - $sortField = $this->mapSortField($sortBy, $optionSpecs, $relationSpecs); + $effectiveMenuindexSql = CategoryProductMenuindexService::effectiveMenuindexSqlForCategories($scopeCategoryIds); + $sortField = $this->mapSortField($sortBy, $optionSpecs, $relationSpecs, $effectiveMenuindexSql); $c->sortby($sortField, $sortDir); $c->limit($limit, $start); @@ -80,9 +86,10 @@ public function getPage( foreach ($relationSpecs as $spec) { $selectParts[] = $spec->selectExpression(); } + $selectParts[] = "{$effectiveMenuindexSql} AS effective_menuindex"; // xPDOQuery::select() declares string, accepts both at runtime but PHPStan is strict. $c->select(implode(', ', $selectParts)); - if ($optionSpecs !== []) { + if ($optionSpecs !== [] || count($scopeCategoryIds) > 1) { $c->groupby('msProduct.id'); } @@ -114,8 +121,12 @@ private function aggregateOptionValueSql(string $alias): string * @param list $optionSpecs * @param list $relationSpecs */ - private function mapSortField(string $sortBy, array $optionSpecs, array $relationSpecs): string - { + private function mapSortField( + string $sortBy, + array $optionSpecs, + array $relationSpecs, + string $effectiveMenuindexSql, + ): string { foreach ($optionSpecs as $spec) { if ($spec->fieldName === $sortBy) { return $this->aggregateOptionValueSql($spec->alias); @@ -126,7 +137,10 @@ private function mapSortField(string $sortBy, array $optionSpecs, array $relatio return $spec->sortExpression(); } } - $productFields = ['id', 'pagetitle', 'menuindex', 'published', 'createdon', 'editedon']; + if ($sortBy === 'menuindex') { + return $effectiveMenuindexSql; + } + $productFields = ['id', 'pagetitle', 'published', 'createdon', 'editedon']; if (in_array($sortBy, $productFields, true)) { return "msProduct.{$sortBy}"; } @@ -139,13 +153,13 @@ private function mapSortField(string $sortBy, array $optionSpecs, array $relatio } /** - * @param list $optionSpecs - * @param list $relationSpecs + * @param list $scopeCategoryIds + * @param list $optionSpecs + * @param list $relationSpecs */ private function buildProductListQuery( - int $categoryId, + array $scopeCategoryIds, array $params, - bool $nested, array $optionSpecs, array $relationSpecs, ): xPDOQuery { @@ -167,15 +181,17 @@ private function buildProductListQuery( $c->leftJoin($spec->modelClass, $spec->alias, $spec->joinCondition()); } + $memberAlias = CategoryProductMenuindexService::MEMBER_JOIN_ALIAS; + $c->leftJoin( + msCategoryMember::class, + $memberAlias, + CategoryProductMenuindexService::memberJoinOnCategories($scopeCategoryIds, $memberAlias) + ); + $c->where(['msProduct.class_key' => msProduct::class]); $scopeService = $this->getCategoryProductScopeService(); - if ($nested) { - $categoryIds = $this->treeService()->productParentIds($categoryId, true); - $scopeService->applyProductCategoryScope($c, $categoryIds); - } else { - $scopeService->applyProductCategoryScope($c, [$categoryId]); - } + $scopeService->applyProductCategoryScope($c, $scopeCategoryIds); if ($query !== '') { $c->where([ @@ -308,7 +324,7 @@ private function formatProductRow( 'longtitle' => $row['longtitle'] ?? '', 'alias' => $row['alias'] ?? '', 'parent' => (int) ($row['parent'] ?? 0), - 'menuindex' => (int) ($row['menuindex'] ?? 0), + 'menuindex' => (int) ($row['effective_menuindex'] ?? $row['menuindex'] ?? 0), 'published' => (bool) ($row['published'] ?? false), 'deleted' => (bool) ($row['deleted'] ?? false), 'hidemenu' => (bool) ($row['hidemenu'] ?? false), diff --git a/core/components/minishop3/src/Services/Product/ProductCatalogService.php b/core/components/minishop3/src/Services/Product/ProductCatalogService.php index 652ba434c..6c8876d18 100644 --- a/core/components/minishop3/src/Services/Product/ProductCatalogService.php +++ b/core/components/minishop3/src/Services/Product/ProductCatalogService.php @@ -6,7 +6,9 @@ use MiniShop3\Model\msProduct; use MiniShop3\Model\msProductData; +use MiniShop3\Model\msCategoryMember; use MiniShop3\Services\Catalog\CatalogQuery; +use MiniShop3\Services\Category\CategoryProductMenuindexService; use MiniShop3\Services\Category\CategoryProductScopeService; use MiniShop3\Services\Option\OptionService; use MODX\Revolution\modX; @@ -293,7 +295,7 @@ public function getList(array $params): array $listQuery = $this->buildListQuery($params, $filters); $this->applyListSelect($listQuery, $includeContent); - $this->applySort($listQuery, $params); + $this->applySort($listQuery, $params, $filters); $listQuery->limit($limit, $offset); /** @var array $products */ @@ -487,12 +489,73 @@ private function prefetchAndAttachProductData(array $products): array /** * @param array $params */ - private function applySort(xPDOQuery $query, array $params): void + private function applySort(xPDOQuery $query, array $params, ProductCatalogFilterSpec $filters): void { + $sortKey = strtolower(trim((string) ($params['sort'] ?? 'menuindex'))); + if ($sortKey === 'menuindex') { + $categoryIds = $this->resolveMenuindexCategoryIds($params, $filters); + if ($categoryIds !== []) { + $this->applyEffectiveMenuindexSort($query, $categoryIds, $params, $filters); + + return; + } + } + [$sortField, $dir] = self::resolveSort($params); $query->sortby($sortField, $dir); } + /** + * @param array $params + * @return list + */ + public function resolveMenuindexCategoryIds(array $params, ProductCatalogFilterSpec $filters): array + { + if ($filters->hasParents()) { + $depth = $filters->nested ? ProductCatalogFilterApplier::NESTED_DEPTH : 0; + $parentsCsv = implode(',', $filters->parentIds); + + return $this->categoryScopeService()->resolveCategoryIdsFromParents($parentsCsv, $depth); + } + + $parent = (int) ($params['parent'] ?? $params['category'] ?? 0); + if ($parent > 0) { + return [$parent]; + } + + return []; + } + + /** + * @param list $categoryIds + * @param array $params + */ + private function applyEffectiveMenuindexSort( + xPDOQuery $query, + array $categoryIds, + array $params, + ProductCatalogFilterSpec $filters, + ): void { + $memberAlias = CategoryProductMenuindexService::MEMBER_JOIN_ALIAS; + $sortSql = CategoryProductMenuindexService::applyMemberJoin($query, $categoryIds); + + [, $dir] = self::resolveSort($params); + + if (count($categoryIds) > 1 || $filters->options !== []) { + $query->groupby('msProduct.id'); + } + + $query->sortby($sortSql, $dir); + } + + private function categoryScopeService(): CategoryProductScopeService + { + /** @var CategoryProductScopeService $scope */ + $scope = $this->modx->services->get('ms3_category_product_scope'); + + return $scope; + } + /** * @param list $productIds * @return array> diff --git a/core/components/minishop3/src/Services/Product/ProductCategoryMembershipWriter.php b/core/components/minishop3/src/Services/Product/ProductCategoryMembershipWriter.php index 9fba052a1..ddaa996e4 100644 --- a/core/components/minishop3/src/Services/Product/ProductCategoryMembershipWriter.php +++ b/core/components/minishop3/src/Services/Product/ProductCategoryMembershipWriter.php @@ -6,6 +6,7 @@ use MiniShop3\Model\msCategoryMember; use MiniShop3\Model\msProductData; +use MiniShop3\Services\Category\CategoryProductMenuindexService; use MODX\Revolution\modX; /** @@ -15,17 +16,19 @@ class ProductCategoryMembershipWriter { use ProductDataExplicitFieldsTrait; - protected modX $modx; + private CategoryProductMenuindexService $menuindexService; - public function __construct(modX $modx) - { - $this->modx = $modx; + public function __construct( + private modX $modx, + ) { + $this->menuindexService = new CategoryProductMenuindexService($modx); } /** * Save additional product categories. * * If `categories` was not sent, leave msCategoryMember untouched. + * Preserves menuindex for kept pairs; new members get MAX(menuindex)+1 in that category. */ public function saveCategories(msProductData $productData): void { @@ -34,33 +37,59 @@ public function saveCategories(msProductData $productData): void return; } - $productId = $productData->get('id'); - $categories = $this->normalizeList($fields['categories']); + $productId = (int) $productData->get('id'); + $desiredCategoryIds = $this->normalizeCategoryIds($fields['categories']); + $desiredCategorySet = array_flip($desiredCategoryIds); - $this->modx->removeCollection(msCategoryMember::class, ['product_id' => $productId]); + /** @var array $existingByCategory */ + $existingByCategory = []; + /** @var msCategoryMember $member */ + foreach ($this->modx->getCollection(msCategoryMember::class, ['product_id' => $productId]) as $member) { + $existingByCategory[(int) $member->get('category_id')] = $member; + } - foreach ($categories as $categoryId) { - if (empty($categoryId) || !is_numeric($categoryId)) { + foreach ($existingByCategory as $categoryId => $member) { + if (!isset($desiredCategorySet[$categoryId])) { + $member->remove(); + } + } + + foreach ($desiredCategoryIds as $categoryId) { + if (array_key_exists($categoryId, $existingByCategory)) { continue; } /** @var msCategoryMember $member */ $member = $this->modx->newObject(msCategoryMember::class); $member->set('product_id', $productId); - $member->set('category_id', (int)$categoryId); + $member->set('category_id', $categoryId); + $member->set('menuindex', $this->menuindexService->getNextMemberMenuindex($categoryId)); $member->save(); } } /** - * @return list + * @return list */ - private function normalizeList(mixed $value): array + private function normalizeCategoryIds(mixed $value): array { if (is_string($value)) { $value = json_decode($value, true); } - return is_array($value) ? $value : []; + if (!is_array($value)) { + return []; + } + + $ids = []; + foreach ($value as $categoryId) { + if (empty($categoryId) || !is_numeric($categoryId)) { + continue; + } + + $ids[] = (int) $categoryId; + } + + return array_values(array_unique($ids)); } } diff --git a/core/components/minishop3/tests/CategoryProductMenuindexServiceTest.php b/core/components/minishop3/tests/CategoryProductMenuindexServiceTest.php new file mode 100644 index 000000000..3794daded --- /dev/null +++ b/core/components/minishop3/tests/CategoryProductMenuindexServiceTest.php @@ -0,0 +1,86 @@ +getNextMemberMenuindex(99), 'empty category next is 0'); + +$menuindexModx->members = [ + ['product_id' => 1, 'category_id' => 10, 'menuindex' => 4], + ['product_id' => 2, 'category_id' => 10, 'menuindex' => 7], +]; +$assertSame(8, $menuindexService->getNextMemberMenuindex(10), 'next after member max'); + +$menuindexModx->nativeProducts = [ + ['parent' => 11, 'menuindex' => 12], +]; +$menuindexModx->members = []; +$assertSame(13, $menuindexService->getNextMemberMenuindex(11), 'next after native max'); + +$menuindexModx->nativeProducts = [ + ['parent' => 12, 'menuindex' => 3], +]; +$menuindexModx->members = [ + ['product_id' => 5, 'category_id' => 12, 'menuindex' => 9], +]; +$assertSame(10, $menuindexService->getNextMemberMenuindex(12), 'next after greater of native and member'); + +fwrite(STDOUT, "OK: CategoryProductMenuindexServiceTest\n"); diff --git a/core/components/minishop3/tests/CategoryProductScopeServiceTest.php b/core/components/minishop3/tests/CategoryProductScopeServiceTest.php index d11be262b..3af095bdc 100644 --- a/core/components/minishop3/tests/CategoryProductScopeServiceTest.php +++ b/core/components/minishop3/tests/CategoryProductScopeServiceTest.php @@ -11,6 +11,7 @@ require __DIR__ . '/stubs/ModxStub.php'; require __DIR__ . '/stubs/StubMsProduct.php'; require __DIR__ . '/stubs/CategoryProductScopeModxStub.php'; +require __DIR__ . '/stubs/StubMsCategoryMember.php'; require __DIR__ . '/../vendor/autoload.php'; use MiniShop3\Model\msProduct; @@ -121,4 +122,18 @@ 'admin scope parent or member ids' ); +$modx->products = [ + ['id' => 30, 'parent' => 2], +]; +$modx->members = [ + ['product_id' => 30, 'category_id' => 1, 'menuindex' => 4], +]; +$scopeWithMember = new CategoryProductScopeService($modx); +$memberProduct = $scopeWithMember->findInCategory(1, 30, false); +if (!$memberProduct instanceof StubMsProduct) { + $fail('member link should resolve product in category scope'); +} +$assertSame(true, $scopeWithMember->canReorderInCategory(30, 1), 'member can reorder in category'); +$assertSame(false, $scopeWithMember->canReorderInCategory(30, 99), 'member cannot reorder outside category'); + fwrite(STDOUT, "OK: CategoryProductScopeServiceTest\n"); diff --git a/core/components/minishop3/tests/CategoryProductsControllerScopeTest.php b/core/components/minishop3/tests/CategoryProductsControllerScopeTest.php index b2a1207ff..53f4f54fe 100644 --- a/core/components/minishop3/tests/CategoryProductsControllerScopeTest.php +++ b/core/components/minishop3/tests/CategoryProductsControllerScopeTest.php @@ -12,6 +12,7 @@ require __DIR__ . '/stubs/StubMsProduct.php'; require __DIR__ . '/stubs/StubMsCategory.php'; require __DIR__ . '/stubs/CategoryProductScopeModxStub.php'; +require __DIR__ . '/stubs/StubMsCategoryMember.php'; require __DIR__ . '/../vendor/autoload.php'; use MiniShop3\Controllers\Api\Manager\CategoryProductsController; @@ -90,6 +91,39 @@ $assertSame(true, $sort['success'] ?? null, 'sort success'); $assertSame(1, $sort['data']['updated'] ?? null, 'sort updated count'); +// sort member product: updates link menuindex only, not resource menuindex (#625) +$modx->products = [ + ['id' => 999, 'parent' => 2, 'published' => 0], + ['id' => 100, 'parent' => 1, 'published' => 0, 'menuindex' => 5], + ['id' => 101, 'parent' => 2, 'published' => 0], + ['id' => 150, 'parent' => 2, 'published' => 0, 'menuindex' => 1], + ['id' => 200, 'parent' => 1, 'published' => 0, 'policies' => ['save' => false]], +]; +$modx->members = [ + ['product_id' => 150, 'category_id' => 1, 'menuindex' => 2], +]; +$memberSort = $controller->sort([ + 'id' => 1, + 'items' => [['id' => 150, 'menuindex' => 9]], +]); +$assertSame(true, $memberSort['success'] ?? null, 'member sort success'); +$assertSame(1, $memberSort['data']['updated'] ?? null, 'member sort updated'); +$product150Menuindex = null; +foreach ($modx->products as $row) { + if ((int) $row['id'] === 150) { + $product150Menuindex = (int) ($row['menuindex'] ?? 0); + break; + } +} +$assertSame(1, $product150Menuindex, 'native resource menuindex unchanged'); +$memberMenuindex = null; +foreach ($modx->members as $row) { + if ((int) $row['product_id'] === 150 && (int) $row['category_id'] === 1) { + $memberMenuindex = (int) ($row['menuindex'] ?? 0); + } +} +$assertSame(9, $memberMenuindex, 'member menuindex updated in category B'); + // updateProductData: in-scope product with document save policy denied → 403 (#473 pattern) $modx->getObjectCalls = []; $aclDenied = $controller->updateProductData([ diff --git a/core/components/minishop3/tests/ProductCatalogServiceTest.php b/core/components/minishop3/tests/ProductCatalogServiceTest.php index 90e36054e..eefd2f6c7 100644 --- a/core/components/minishop3/tests/ProductCatalogServiceTest.php +++ b/core/components/minishop3/tests/ProductCatalogServiceTest.php @@ -125,5 +125,8 @@ 'images flag keeps allowlisted gallery rows' ); +$effectiveSingle = \MiniShop3\Services\Category\CategoryProductMenuindexService::effectiveMenuindexSql(42); +$assertSame(true, str_contains($effectiveSingle, 'msProduct.parent = 42'), 'catalog effective menuindex uses category context'); + fwrite(STDOUT, "OK ProductCatalogServiceTest\n"); exit(0); diff --git a/core/components/minishop3/tests/ProductCategoryMembershipWriterTest.php b/core/components/minishop3/tests/ProductCategoryMembershipWriterTest.php new file mode 100644 index 000000000..6d76e111b --- /dev/null +++ b/core/components/minishop3/tests/ProductCategoryMembershipWriterTest.php @@ -0,0 +1,110 @@ + */ + public array $_fields = []; + + public function get($key, $format = null, $formatString = '') + { + if ($key === 'id') { + return 100; + } + + return parent::get($key, $format, $formatString); + } +} + +$fail = static function (string $message): never { + fwrite(STDERR, "FAIL: {$message}\n"); + exit(1); +}; + +$assertSame = static function ($expected, $actual, string $case) use ($fail): void { + if ($actual !== $expected) { + $fail($case . ': expected ' . var_export($expected, true) . ', got ' . var_export($actual, true)); + } +}; + +$modx = new ProductCategoryMembershipModxStub(); +$modx->members = [ + ['product_id' => 100, 'category_id' => 2, 'menuindex' => 7], + ['product_id' => 100, 'category_id' => 3, 'menuindex' => 4], +]; +$modx->nativeProducts = [ + ['parent' => 5, 'menuindex' => 2], +]; + +$writer = new ProductCategoryMembershipWriter($modx); +$xpdo = new class extends \xPDO\xPDO { + public object $services; + + public function __construct() + { + $this->services = new class { + public function has(string $key): bool + { + return false; + } + + public function get(string $key): null + { + return null; + } + }; + } +}; + +$setExplicitCategories = static function (WriterTestProductData $productData, array $categories): void { + $productData->_fields = [ + 'id' => 100, + 'categories' => $categories, + ]; +}; + +$productData = new WriterTestProductData($xpdo); +$setExplicitCategories($productData, [2, 3, 5]); + +$writer->saveCategories($productData); + +$byCategory = []; +foreach ($modx->members as $row) { + $byCategory[(int) $row['category_id']] = (int) $row['menuindex']; +} + +$assertSame(7, $byCategory[2] ?? null, 'kept menuindex for category 2'); +$assertSame(4, $byCategory[3] ?? null, 'kept menuindex for category 3'); +$assertSame(3, $byCategory[5] ?? null, 'new member gets next menuindex after native product in category 5'); + +$productData2 = new WriterTestProductData($xpdo); +$setExplicitCategories($productData2, [2]); + +$writer->saveCategories($productData2); + +$remainingCategories = array_map( + static fn(array $row): int => (int) $row['category_id'], + $modx->members +); +sort($remainingCategories); +$assertSame([2], $remainingCategories, 'removed unlisted categories'); +$assertSame(7, $modx->members[0]['menuindex'], 'menuindex still preserved after shrink'); + +fwrite(STDOUT, "OK: ProductCategoryMembershipWriterTest\n"); diff --git a/core/components/minishop3/tests/stubs/CategoryProductScopeModxStub.php b/core/components/minishop3/tests/stubs/CategoryProductScopeModxStub.php index 5b3fc59db..f39c44dcd 100644 --- a/core/components/minishop3/tests/stubs/CategoryProductScopeModxStub.php +++ b/core/components/minishop3/tests/stubs/CategoryProductScopeModxStub.php @@ -5,6 +5,7 @@ namespace MiniShop3\Tests\Stubs; use MiniShop3\Model\msCategory; +use MiniShop3\Model\msCategoryMember; use MiniShop3\Model\msProduct; use MODX\Revolution\modX; @@ -16,9 +17,12 @@ class CategoryProductScopeModxStub extends modX /** @var object|null */ public $lexicon; - /** @var list}> */ + /** @var list}> */ public array $products = []; + /** @var list */ + public array $members = []; + /** @var list */ public array $categories = []; @@ -74,6 +78,19 @@ public function getObject($className = '', $criteria = null, $cacheFlag = true) { $this->getObjectCalls[] = ['class' => $className, 'criteria' => $criteria]; + if ($className === msCategoryMember::class && is_array($criteria)) { + $productId = (int) ($criteria['product_id'] ?? 0); + $categoryId = (int) ($criteria['category_id'] ?? 0); + + foreach ($this->members as $row) { + if ((int) $row['product_id'] === $productId && (int) $row['category_id'] === $categoryId) { + return new StubMsCategoryMember($row, $this); + } + } + + return null; + } + if ($className === msCategory::class) { $categoryId = is_array($criteria) ? (int) ($criteria['id'] ?? 0) @@ -113,6 +130,18 @@ public function getObject($className = '', $criteria = null, $cacheFlag = true) } } + if ($parentId > 0) { + return null; + } + + if ($productId > 0) { + foreach ($this->products as $row) { + if ((int) $row['id'] === $productId) { + return new StubMsProduct($row, $row['policies'] ?? null); + } + } + } + return null; } diff --git a/core/components/minishop3/tests/stubs/ProductCategoryMembershipModxStub.php b/core/components/minishop3/tests/stubs/ProductCategoryMembershipModxStub.php new file mode 100644 index 000000000..2a09abeaa --- /dev/null +++ b/core/components/minishop3/tests/stubs/ProductCategoryMembershipModxStub.php @@ -0,0 +1,227 @@ + */ + public array $members = []; + + /** @var list */ + public array $nativeProducts = []; + + public function __construct() + { + parent::__construct(); + } + + /** + * @param class-string $className + * @return list + */ + public function getCollection($className, $criteria = null, $cacheFlag = false) + { + if ($className !== msCategoryMember::class || !is_array($criteria)) { + return []; + } + + $productId = (int) ($criteria['product_id'] ?? 0); + + return array_map( + fn(array $row): StubMsCategoryMemberForWriter => new StubMsCategoryMemberForWriter($row, $this), + array_values(array_filter( + $this->members, + static fn(array $row): bool => (int) $row['product_id'] === $productId + )) + ); + } + + /** + * @param class-string $className + */ + public function newObject($className = '', $fields = []) + { + if ($className !== msCategoryMember::class) { + return null; + } + + return new StubMsCategoryMemberForWriter([ + 'product_id' => 0, + 'category_id' => 0, + 'menuindex' => 0, + ], $this); + } + + public function newQuery($class = '', $criteria = null) + { + return new MembershipWriterQueryStub($this, (string) $class); + } +} + +class StubMsCategoryMemberForWriter +{ + /** @var array */ + private array $data; + + private ?ProductCategoryMembershipModxStub $modx; + + private bool $removed = false; + + /** + * @param array $data + */ + public function __construct(array $data, ?ProductCategoryMembershipModxStub $modx = null) + { + $this->data = $data; + $this->modx = $modx; + } + + public function get(string $key): mixed + { + return $this->data[$key] ?? null; + } + + public function set(string $key, mixed $value): void + { + $this->data[$key] = $value; + } + + public function save(): bool + { + if ($this->removed || $this->modx === null) { + return false; + } + + $productId = (int) $this->data['product_id']; + $categoryId = (int) $this->data['category_id']; + $found = false; + + foreach ($this->modx->members as $index => $row) { + if ((int) $row['product_id'] === $productId && (int) $row['category_id'] === $categoryId) { + $this->modx->members[$index] = [ + 'product_id' => $productId, + 'category_id' => $categoryId, + 'menuindex' => (int) ($this->data['menuindex'] ?? 0), + ]; + $found = true; + break; + } + } + + if (!$found) { + $this->modx->members[] = [ + 'product_id' => $productId, + 'category_id' => $categoryId, + 'menuindex' => (int) ($this->data['menuindex'] ?? 0), + ]; + } + + return true; + } + + public function remove(): void + { + if ($this->modx === null) { + return; + } + + $this->removed = true; + $productId = (int) $this->data['product_id']; + $categoryId = (int) $this->data['category_id']; + $this->modx->members = array_values(array_filter( + $this->modx->members, + static fn(array $row): bool => !((int) $row['product_id'] === $productId && (int) $row['category_id'] === $categoryId) + )); + } +} + +class MembershipWriterQueryStub +{ + /** @var array */ + private array $where = []; + + /** @var MembershipWriterStatementStub|null */ + public $stmt; + + public function __construct( + private ProductCategoryMembershipModxStub $modxStub, + private string $class, + ) { + } + + public function where($conditions = '', $conj = '', $binding = null) + { + if (is_array($conditions)) { + $this->where = array_merge($this->where, $conditions); + } + + return $this; + } + + public function select($columns = '*') + { + return $this; + } + + public function prepare($bindings = null) + { + $this->stmt = new MembershipWriterStatementStub($this->modxStub, $this->class, $this->where); + + return true; + } +} + +class MembershipWriterStatementStub +{ + public function __construct( + private ProductCategoryMembershipModxStub $modxStub, + private string $class, + /** @var array */ + private array $where, + ) { + } + + public function execute($params = null) + { + return true; + } + + public function fetchColumn($column = 0) + { + if ($this->class === msCategoryMember::class) { + $categoryId = (int) ($this->where['category_id'] ?? 0); + $max = -1; + foreach ($this->modxStub->members as $row) { + if ((int) $row['category_id'] === $categoryId) { + $max = max($max, (int) $row['menuindex']); + } + } + + return $max >= 0 ? $max : false; + } + + if ($this->class === msProduct::class) { + $parent = (int) ($this->where['parent'] ?? 0); + $max = -1; + foreach ($this->modxStub->nativeProducts as $row) { + if ((int) $row['parent'] === $parent) { + $max = max($max, (int) $row['menuindex']); + } + } + + return $max >= 0 ? $max : false; + } + + return false; + } +} diff --git a/core/components/minishop3/tests/stubs/StubMsCategoryMember.php b/core/components/minishop3/tests/stubs/StubMsCategoryMember.php new file mode 100644 index 000000000..4cac1df42 --- /dev/null +++ b/core/components/minishop3/tests/stubs/StubMsCategoryMember.php @@ -0,0 +1,51 @@ + */ + private array $data; + + private CategoryProductScopeModxStub $modx; + + /** + * @param array $data + */ + public function __construct(array $data, CategoryProductScopeModxStub $modx) + { + $this->data = $data; + $this->modx = $modx; + } + + public function get(string $key): mixed + { + return $this->data[$key] ?? null; + } + + public function set(string $key, mixed $value): void + { + $this->data[$key] = $value; + } + + public function save(): bool + { + $productId = (int) $this->data['product_id']; + $categoryId = (int) $this->data['category_id']; + + foreach ($this->modx->members as $index => $row) { + if ((int) $row['product_id'] === $productId && (int) $row['category_id'] === $categoryId) { + $this->modx->members[$index] = $this->data; + + return true; + } + } + + return false; + } +} From 43ca8b2ae6fae3a65e56040073d2ec8879ed2d99 Mon Sep 17 00:00:00 2001 From: Ibochkarev <2138260+Ibochkarev@users.noreply.github.com> Date: Sun, 6 Sep 2026 08:27:29 +0200 Subject: [PATCH 11/19] =?UTF-8?q?PR=20#629:=20fix(settings):=20=D1=81?= =?UTF-8?q?=D0=B2=D1=8F=D0=B7=D0=BA=D0=B0=20=D1=81=D0=BF=D0=BE=D1=81=D0=BE?= =?UTF-8?q?=D0=B1=D0=B0=20=D0=BE=D0=BF=D0=BB=D0=B0=D1=82=D1=8B=20=D1=81=20?= =?UTF-8?q?=D0=B4=D0=BE=D1=81=D1=82=D0=B0=D0=B2=D0=BA=D0=BE=D0=B9=20=D0=B2?= =?UTF-8?q?=20=D0=BC=D0=B5=D0=BD=D0=B5=D0=B4=D0=B6=D0=B5=D1=80=D0=B5?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit https://github.com/modx-pro/MiniShop3/pull/629 --- .../Reference/Ms3ReferenceCrudService.php | 4 +- .../Ms3ReferenceCrudServiceAddLinkTest.php | 165 ++++++++++++++++++ 2 files changed, 168 insertions(+), 1 deletion(-) create mode 100644 core/components/minishop3/tests/Unit/Services/Reference/Ms3ReferenceCrudServiceAddLinkTest.php diff --git a/core/components/minishop3/src/Services/Reference/Ms3ReferenceCrudService.php b/core/components/minishop3/src/Services/Reference/Ms3ReferenceCrudService.php index 13026b703..7836f410c 100644 --- a/core/components/minishop3/src/Services/Reference/Ms3ReferenceCrudService.php +++ b/core/components/minishop3/src/Services/Reference/Ms3ReferenceCrudService.php @@ -315,7 +315,9 @@ public function addLink(array $data = []): array } $member = $this->modx->newObject(msDeliveryMember::class); - $member->fromArray($criteria); + // Composite PK: xPDO fromArray() skips PK fields unless setPrimaryKeys is true. + $member->set($this->config->memberOwnFk, $ownId); + $member->set($this->config->memberPeerFk, $peerId); if (!$member->save()) { return Response::error( diff --git a/core/components/minishop3/tests/Unit/Services/Reference/Ms3ReferenceCrudServiceAddLinkTest.php b/core/components/minishop3/tests/Unit/Services/Reference/Ms3ReferenceCrudServiceAddLinkTest.php new file mode 100644 index 000000000..fb5ddaeb7 --- /dev/null +++ b/core/components/minishop3/tests/Unit/Services/Reference/Ms3ReferenceCrudServiceAddLinkTest.php @@ -0,0 +1,165 @@ +modxForAddLink($saved), + ReferenceResourceConfig::forDeliveries() + ); + + $result = $service->addLink(['delivery_id' => 1, 'payment_id' => 42]); + + self::assertTrue($result['success'] ?? false); + self::assertSame([ + ['delivery_id' => 1, 'payment_id' => 42], + ], $saved); + } + + public function testAddLinkPersistsCompositePrimaryKeysForPaymentDelivery(): void + { + $saved = []; + $service = new Ms3ReferenceCrudService( + $this->modxForAddLink($saved), + ReferenceResourceConfig::forPayments() + ); + + $result = $service->addLink(['payment_id' => 5, 'delivery_id' => 9]); + + self::assertTrue($result['success'] ?? false); + self::assertSame([ + ['delivery_id' => 9, 'payment_id' => 5], + ], $saved); + } + + public function testAddLinkSaveFailureReturnsError(): void + { + $saved = []; + $service = new Ms3ReferenceCrudService( + $this->modxForAddLink($saved, false), + ReferenceResourceConfig::forDeliveries() + ); + + $result = $service->addLink(['delivery_id' => 1, 'payment_id' => 42]); + + self::assertFalse($result['success'] ?? true); + self::assertSame('Failed to add payment to delivery', $result['message'] ?? null); + self::assertSame([], $saved); + } + + /** + * @param list $saved + */ + private function modxForAddLink(array &$saved, bool $saveOk = true): modX + { + return new class ($saved, $saveOk) extends modX { + /** + * @param list $saved + */ + public function __construct( + private array &$saved, + private bool $saveOk, + ) { + parent::__construct(); + } + + public function getObject($className, $criteria = null, $cacheFlag = true) + { + if ($className !== msDeliveryMember::class || !is_array($criteria)) { + return null; + } + + foreach ($this->saved as $row) { + if ( + $row['delivery_id'] === (int) ($criteria['delivery_id'] ?? 0) + && $row['payment_id'] === (int) ($criteria['payment_id'] ?? 0) + ) { + return new \stdClass(); + } + } + + return null; + } + + public function newObject($className, $fields = []) + { + return new DeliveryMemberSaveProbe($this->saved, $this->saveOk); + } + }; + } +} + +/** + * Records composite PK fields. fromArray() skips PK keys unless $setPrimaryKeys (xPDO). + */ +final class DeliveryMemberSaveProbe +{ + /** @var array */ + private array $fields = []; + + /** + * @param list $saved + */ + public function __construct( + private array &$saved, + private bool $saveOk, + ) { + } + + public function set($key, $value = null): bool + { + $this->fields[(string) $key] = $value; + + return true; + } + + public function fromArray($fields, $keyPrefix = '', $setPrimaryKeys = false): bool + { + $pk = ['delivery_id', 'payment_id']; + foreach ((array) $fields as $key => $value) { + if (!$setPrimaryKeys && in_array((string) $key, $pk, true)) { + continue; + } + $this->fields[(string) $key] = $value; + } + + return true; + } + + public function save(): bool + { + $row = [ + 'delivery_id' => (int) ($this->fields['delivery_id'] ?? 0), + 'payment_id' => (int) ($this->fields['payment_id'] ?? 0), + ]; + if (!$this->saveOk || $row['delivery_id'] <= 0 || $row['payment_id'] <= 0) { + return false; + } + + $this->saved[] = $row; + + return true; + } +} From f8ffcc51d8e80a3f42de66a4ddade80634c07bf3 Mon Sep 17 00:00:00 2001 From: Ibochkarev <2138260+Ibochkarev@users.noreply.github.com> Date: Sun, 6 Sep 2026 08:27:29 +0200 Subject: [PATCH 12/19] =?UTF-8?q?PR=20#631:=20fix(vue):=20=D1=83=D0=B4?= =?UTF-8?q?=D0=B0=D0=BB=D0=B5=D0=BD=D0=B8=D0=B5=20=D0=B8=D0=B7=20=D0=B3?= =?UTF-8?q?=D1=80=D0=B8=D0=B4=D0=B0=20=D0=BD=D0=B0=D1=81=D1=82=D1=80=D0=BE?= =?UTF-8?q?=D0=B5=D0=BA=20=D0=B1=D0=B5=D0=B7=20=D0=B4=D0=B2=D0=BE=D0=B9?= =?UTF-8?q?=D0=BD=D0=BE=D0=B3=D0=BE=20confirm?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit https://github.com/modx-pro/MiniShop3/pull/631 --- vueManager/src/components/DeliveriesGrid.vue | 81 +++++++--------- vueManager/src/components/LinksGrid.vue | 67 ++++++-------- vueManager/src/components/PaymentsGrid.vue | 81 +++++++--------- vueManager/src/components/StatusesGrid.vue | 67 ++++++-------- vueManager/src/components/VendorsGrid.vue | 92 +++++++------------ vueManager/src/utils/gridDeleteAction.js | 39 ++++++++ vueManager/src/utils/gridDeleteAction.test.js | 23 +++++ .../tests/settingsConfirmGroups.test.js | 21 ++++- .../tests/settingsGridDeleteHandlers.test.js | 38 ++++++++ 9 files changed, 268 insertions(+), 241 deletions(-) create mode 100644 vueManager/src/utils/gridDeleteAction.js create mode 100644 vueManager/src/utils/gridDeleteAction.test.js create mode 100644 vueManager/tests/settingsGridDeleteHandlers.test.js diff --git a/vueManager/src/components/DeliveriesGrid.vue b/vueManager/src/components/DeliveriesGrid.vue index 7b9337987..041d5b2ec 100644 --- a/vueManager/src/components/DeliveriesGrid.vue +++ b/vueManager/src/components/DeliveriesGrid.vue @@ -18,7 +18,6 @@ import Tabs from 'primevue/tabs' import Textarea from 'primevue/textarea' import Toast from 'primevue/toast' import ToggleSwitch from 'primevue/toggleswitch' -import { useConfirm } from 'primevue/useconfirm' import { useToast } from 'primevue/usetoast' import { computed, onMounted, ref } from 'vue' import draggable from 'vuedraggable' @@ -31,16 +30,20 @@ import { useSortableList } from '../composables/useSortableList.js' import request from '../request.js' import { resolveAddCostPriceBadgeKind } from '../utils/addCostPriceBadgeKind.js' import { formatValue, getDisplayName, normalizeImagePath } from '../utils/displayFormatters.js' +import { applyDeleteConfirmDefaults, gridDeleteAction } from '../utils/gridDeleteAction.js' import ActionsColumn from './ActionsColumn.vue' import FileBrowser from './FileBrowser.vue' import ValidationRulesEditor from './ValidationRulesEditor.vue' const toast = useToast() -const confirm = useConfirm() const { _ } = useLexicon() const CONFIRM_GROUP = 'settings-deliveries' +const DELIVERY_GRID_DELETE_ACTION = gridDeleteAction({ + confirmMessage: 'delivery_delete_confirm_message', +}) + // Bulk selection const { selectedItems, @@ -200,14 +203,7 @@ function getFallbackColumns() { width: '7.5rem', actions: [ { name: 'edit', handler: 'edit', icon: 'pi-pencil', label: 'edit' }, - { - name: 'delete', - handler: 'delete', - icon: 'pi-trash', - label: 'delete', - severity: 'danger', - confirm: false, - }, + { ...DELIVERY_GRID_DELETE_ACTION }, ], }, ] @@ -342,38 +338,27 @@ async function saveDelivery() { } /** - * Delete delivery with confirmation + * Delete delivery (called after confirmation in ActionsColumn / useActions) */ -function deleteDelivery(delivery) { - confirm.require({ - group: CONFIRM_GROUP, - message: _('delivery_delete_confirm_message').replace('{name}', delivery.name), - header: _('confirm_delete'), - icon: 'pi pi-exclamation-triangle', - acceptLabel: _('delete'), - rejectLabel: _('cancel'), - acceptClass: 'p-button-danger', - accept: async () => { - try { - await request.delete(`/api/mgr/deliveries/${delivery.id}`) - toast.add({ - severity: 'success', - summary: _('success'), - detail: _('delivery_deleted'), - life: 3000, - }) - loadDeliveries() - } catch (error) { - console.error('[DeliveriesGrid] Error deleting delivery:', error) - toast.add({ - severity: 'error', - summary: _('error'), - detail: error.message || _('error_deleting_data'), - life: 5000, - }) - } - }, - }) +async function deleteDelivery(delivery) { + try { + await request.delete(`/api/mgr/deliveries/${delivery.id}`) + toast.add({ + severity: 'success', + summary: _('success'), + detail: _('delivery_deleted'), + life: 3000, + }) + await loadDeliveries() + } catch (error) { + console.error('[DeliveriesGrid] Error deleting delivery:', error) + toast.add({ + severity: 'error', + summary: _('error'), + detail: error.message || _('error_deleting_data'), + life: 5000, + }) + } } /** @@ -391,15 +376,15 @@ function clearFilters() { resetPageAndLoad() } -/** - * Get actions config for ActionsColumn - */ function getActionsConfig(column) { const config = column.actions || [] - return config.map(action => ({ - ...action, - label: _(action.label) || action.label, - })) + return applyDeleteConfirmDefaults( + config.map(action => ({ + ...action, + label: _(action.label) || action.label, + })), + { confirmMessage: 'delivery_delete_confirm_message' } + ) } /** diff --git a/vueManager/src/components/LinksGrid.vue b/vueManager/src/components/LinksGrid.vue index 01891f8c6..1e5ba50b9 100644 --- a/vueManager/src/components/LinksGrid.vue +++ b/vueManager/src/components/LinksGrid.vue @@ -11,7 +11,6 @@ import Paginator from 'primevue/paginator' import Select from 'primevue/select' import Textarea from 'primevue/textarea' import Toast from 'primevue/toast' -import { useConfirm } from 'primevue/useconfirm' import { useToast } from 'primevue/usetoast' import { computed, onMounted, ref } from 'vue' @@ -19,14 +18,18 @@ import { useCrudDialog } from '../composables/useCrudDialog.js' import { useResourceList } from '../composables/useResourceList.js' import { useSelection } from '../composables/useSelection.js' import request from '../request.js' +import { gridDeleteAction } from '../utils/gridDeleteAction.js' import ActionsColumn from './ActionsColumn.vue' const toast = useToast() -const confirm = useConfirm() const { _ } = useLexicon() const CONFIRM_GROUP = 'settings-links' +const LINK_GRID_DELETE_ACTION = gridDeleteAction({ + confirmMessage: 'link_delete_confirm_message', +}) + // Bulk selection const { selectedItems, @@ -146,38 +149,27 @@ async function saveLink() { } /** - * Delete link with confirmation + * Delete link (called after confirmation in ActionsColumn / useActions) */ -function deleteLink(link) { - confirm.require({ - group: CONFIRM_GROUP, - message: _('link_delete_confirm_message').replace('{name}', link.name), - header: _('confirm_delete'), - icon: 'pi pi-exclamation-triangle', - acceptLabel: _('delete'), - rejectLabel: _('cancel'), - acceptClass: 'p-button-danger', - accept: async () => { - try { - await request.delete(`/api/mgr/links/${link.id}`) - toast.add({ - severity: 'success', - summary: _('success'), - detail: _('link_deleted'), - life: 3000, - }) - loadLinks() - } catch (error) { - console.error('[LinksGrid] Error deleting link:', error) - toast.add({ - severity: 'error', - summary: _('error'), - detail: error.message || _('error_deleting_data'), - life: 5000, - }) - } - }, - }) +async function deleteLink(link) { + try { + await request.delete(`/api/mgr/links/${link.id}`) + toast.add({ + severity: 'success', + summary: _('success'), + detail: _('link_deleted'), + life: 3000, + }) + await loadLinks() + } catch (error) { + console.error('[LinksGrid] Error deleting link:', error) + toast.add({ + severity: 'error', + summary: _('error'), + detail: error.message || _('error_deleting_data'), + life: 5000, + }) + } } /** @@ -186,14 +178,7 @@ function deleteLink(link) { function getActionsConfig() { return [ { name: 'edit', handler: 'edit', icon: 'pi-pencil', label: _('edit') }, - { - name: 'delete', - handler: 'delete', - icon: 'pi-trash', - label: _('delete'), - severity: 'danger', - confirm: false, - }, + { ...LINK_GRID_DELETE_ACTION, label: _('delete') }, ] } diff --git a/vueManager/src/components/PaymentsGrid.vue b/vueManager/src/components/PaymentsGrid.vue index 980d6f636..594f09b0a 100644 --- a/vueManager/src/components/PaymentsGrid.vue +++ b/vueManager/src/components/PaymentsGrid.vue @@ -17,7 +17,6 @@ import Tabs from 'primevue/tabs' import Textarea from 'primevue/textarea' import Toast from 'primevue/toast' import ToggleSwitch from 'primevue/toggleswitch' -import { useConfirm } from 'primevue/useconfirm' import { useToast } from 'primevue/usetoast' import { computed, onMounted, ref } from 'vue' import draggable from 'vuedraggable' @@ -30,15 +29,19 @@ import { useSortableList } from '../composables/useSortableList.js' import request from '../request.js' import { resolveAddCostPriceBadgeKind } from '../utils/addCostPriceBadgeKind.js' import { formatValue, getDisplayName, normalizeImagePath } from '../utils/displayFormatters.js' +import { applyDeleteConfirmDefaults, gridDeleteAction } from '../utils/gridDeleteAction.js' import ActionsColumn from './ActionsColumn.vue' import FileBrowser from './FileBrowser.vue' const toast = useToast() -const confirm = useConfirm() const { _ } = useLexicon() const CONFIRM_GROUP = 'settings-payments' +const PAYMENT_GRID_DELETE_ACTION = gridDeleteAction({ + confirmMessage: 'payment_delete_confirm_message', +}) + // Bulk selection const { selectedItems, @@ -175,14 +178,7 @@ function getFallbackColumns() { width: '7.5rem', actions: [ { name: 'edit', handler: 'edit', icon: 'pi-pencil', label: 'edit' }, - { - name: 'delete', - handler: 'delete', - icon: 'pi-trash', - label: 'delete', - severity: 'danger', - confirm: false, - }, + { ...PAYMENT_GRID_DELETE_ACTION }, ], }, ] @@ -310,38 +306,27 @@ async function savePayment() { } /** - * Delete payment with confirmation + * Delete payment (called after confirmation in ActionsColumn / useActions) */ -function deletePayment(payment) { - confirm.require({ - group: CONFIRM_GROUP, - message: _('payment_delete_confirm_message').replace('{name}', payment.name), - header: _('confirm_delete'), - icon: 'pi pi-exclamation-triangle', - acceptLabel: _('delete'), - rejectLabel: _('cancel'), - acceptClass: 'p-button-danger', - accept: async () => { - try { - await request.delete(`/api/mgr/payments/${payment.id}`) - toast.add({ - severity: 'success', - summary: _('success'), - detail: _('payment_deleted'), - life: 3000, - }) - loadPayments() - } catch (error) { - console.error('[PaymentsGrid] Error deleting payment:', error) - toast.add({ - severity: 'error', - summary: _('error'), - detail: error.message || _('error_deleting_data'), - life: 5000, - }) - } - }, - }) +async function deletePayment(payment) { + try { + await request.delete(`/api/mgr/payments/${payment.id}`) + toast.add({ + severity: 'success', + summary: _('success'), + detail: _('payment_deleted'), + life: 3000, + }) + await loadPayments() + } catch (error) { + console.error('[PaymentsGrid] Error deleting payment:', error) + toast.add({ + severity: 'error', + summary: _('error'), + detail: error.message || _('error_deleting_data'), + life: 5000, + }) + } } /** @@ -359,15 +344,15 @@ function clearFilters() { resetPageAndLoad() } -/** - * Get actions config for ActionsColumn - */ function getActionsConfig(column) { const config = column.actions || [] - return config.map(action => ({ - ...action, - label: _(action.label) || action.label, - })) + return applyDeleteConfirmDefaults( + config.map(action => ({ + ...action, + label: _(action.label) || action.label, + })), + { confirmMessage: 'payment_delete_confirm_message' } + ) } /** diff --git a/vueManager/src/components/StatusesGrid.vue b/vueManager/src/components/StatusesGrid.vue index a902c990e..ed7d963a6 100644 --- a/vueManager/src/components/StatusesGrid.vue +++ b/vueManager/src/components/StatusesGrid.vue @@ -9,7 +9,6 @@ import Dialog from 'primevue/dialog' import InputText from 'primevue/inputtext' import Textarea from 'primevue/textarea' import Toast from 'primevue/toast' -import { useConfirm } from 'primevue/useconfirm' import { useToast } from 'primevue/usetoast' import { onMounted, ref } from 'vue' import draggable from 'vuedraggable' @@ -19,14 +18,18 @@ import { useResourceList } from '../composables/useResourceList.js' import { useSelection } from '../composables/useSelection.js' import { useSortableList } from '../composables/useSortableList.js' import request from '../request.js' +import { gridDeleteAction } from '../utils/gridDeleteAction.js' import ActionsColumn from './ActionsColumn.vue' const toast = useToast() -const confirm = useConfirm() const { _ } = useLexicon() const CONFIRM_GROUP = 'settings-statuses' +const STATUS_GRID_DELETE_ACTION = gridDeleteAction({ + confirmMessage: 'status_delete_confirm_message', +}) + // Bulk selection const { selectedItems, @@ -152,38 +155,27 @@ async function saveStatus() { } /** - * Delete status with confirmation + * Delete status (called after confirmation in ActionsColumn / useActions) */ -function deleteStatus(status) { - confirm.require({ - group: CONFIRM_GROUP, - message: _('status_delete_confirm_message').replace('{name}', status.name), - header: _('confirm_delete'), - icon: 'pi pi-exclamation-triangle', - acceptLabel: _('delete'), - rejectLabel: _('cancel'), - acceptClass: 'p-button-danger', - accept: async () => { - try { - await request.delete(`/api/mgr/statuses/${status.id}`) - toast.add({ - severity: 'success', - summary: _('success'), - detail: _('status_deleted'), - life: 3000, - }) - loadStatuses() - } catch (error) { - console.error('[StatusesGrid] Error deleting status:', error) - toast.add({ - severity: 'error', - summary: _('error'), - detail: error.message || _('error_deleting_data'), - life: 5000, - }) - } - }, - }) +async function deleteStatus(status) { + try { + await request.delete(`/api/mgr/statuses/${status.id}`) + toast.add({ + severity: 'success', + summary: _('success'), + detail: _('status_deleted'), + life: 3000, + }) + await loadStatuses() + } catch (error) { + console.error('[StatusesGrid] Error deleting status:', error) + toast.add({ + severity: 'error', + summary: _('error'), + detail: error.message || _('error_deleting_data'), + life: 5000, + }) + } } /** @@ -210,14 +202,7 @@ function selectColor(color) { function getActionsConfig() { return [ { name: 'edit', handler: 'edit', icon: 'pi-pencil', label: _('edit') }, - { - name: 'delete', - handler: 'delete', - icon: 'pi-trash', - label: _('delete'), - severity: 'danger', - confirm: false, - }, + { ...STATUS_GRID_DELETE_ACTION, label: _('delete') }, ] } diff --git a/vueManager/src/components/VendorsGrid.vue b/vueManager/src/components/VendorsGrid.vue index aa556f95d..04bc55265 100644 --- a/vueManager/src/components/VendorsGrid.vue +++ b/vueManager/src/components/VendorsGrid.vue @@ -14,7 +14,6 @@ import TabPanels from 'primevue/tabpanels' import Tabs from 'primevue/tabs' import Textarea from 'primevue/textarea' import Toast from 'primevue/toast' -import { useConfirm } from 'primevue/useconfirm' import { useToast } from 'primevue/usetoast' import { computed, onMounted, ref } from 'vue' import draggable from 'vuedraggable' @@ -25,16 +24,20 @@ import { useSelection } from '../composables/useSelection.js' import { useSortableList } from '../composables/useSortableList.js' import request from '../request.js' import { formatValue, normalizeImagePath } from '../utils/displayFormatters.js' +import { applyDeleteConfirmDefaults, gridDeleteAction } from '../utils/gridDeleteAction.js' import ActionsColumn from './ActionsColumn.vue' import DynamicField from './DynamicField.vue' import FileBrowser from './FileBrowser.vue' const toast = useToast() -const confirm = useConfirm() const { _ } = useLexicon() const CONFIRM_GROUP = 'settings-vendors' +const VENDOR_GRID_DELETE_ACTION = gridDeleteAction({ + confirmMessage: 'vendor_delete_confirm_message', +}) + // Bulk selection const { selectedItems, @@ -309,38 +312,27 @@ async function saveVendor() { } /** - * Delete vendor with confirmation + * Delete vendor (called after confirmation in ActionsColumn / useActions) */ -function deleteVendor(vendor) { - confirm.require({ - group: CONFIRM_GROUP, - message: _('vendor_delete_confirm_message').replace('{name}', vendor.name), - header: _('confirm_delete'), - icon: 'pi pi-exclamation-triangle', - acceptLabel: _('delete'), - rejectLabel: _('cancel'), - acceptClass: 'p-button-danger', - accept: async () => { - try { - await request.delete(`/api/mgr/vendors/${vendor.id}`) - toast.add({ - severity: 'success', - summary: _('success'), - detail: _('vendor_deleted'), - life: 3000, - }) - loadVendors() - } catch (error) { - console.error('[VendorsGrid] Error deleting vendor:', error) - toast.add({ - severity: 'error', - summary: _('error'), - detail: error.message || _('error_deleting_data'), - life: 5000, - }) - } - }, - }) +async function deleteVendor(vendor) { + try { + await request.delete(`/api/mgr/vendors/${vendor.id}`) + toast.add({ + severity: 'success', + summary: _('success'), + detail: _('vendor_deleted'), + life: 3000, + }) + await loadVendors() + } catch (error) { + console.error('[VendorsGrid] Error deleting vendor:', error) + toast.add({ + severity: 'error', + summary: _('error'), + detail: error.message || _('error_deleting_data'), + life: 5000, + }) + } } /** @@ -369,30 +361,22 @@ function onSelectAllChange() { } } -/** - * Get actions config for ActionsColumn - */ function getActionsConfig(column) { // Fallback actions if not configured if (!column.actions || column.actions.length === 0) { return [ { name: 'edit', handler: 'edit', icon: 'pi-pencil', label: _('edit') }, - { - name: 'delete', - handler: 'delete', - icon: 'pi-trash', - label: _('delete'), - severity: 'danger', - confirm: false, - confirmMessage: 'vendor_delete_confirm_message', - }, + { ...VENDOR_GRID_DELETE_ACTION, label: _('delete') }, ] } - return column.actions.map(action => ({ - ...action, - label: _(action.label) || action.label, - })) + return applyDeleteConfirmDefaults( + column.actions.map(action => ({ + ...action, + label: _(action.label) || action.label, + })), + { confirmMessage: 'vendor_delete_confirm_message' } + ) } /** @@ -444,15 +428,7 @@ function getDefaultColumns() { type: 'actions', actions: [ { name: 'edit', handler: 'edit', icon: 'pi-pencil', label: 'edit' }, - { - name: 'delete', - handler: 'delete', - icon: 'pi-trash', - label: 'delete', - severity: 'danger', - confirm: false, - confirmMessage: 'vendor_delete_confirm_message', - }, + { ...VENDOR_GRID_DELETE_ACTION }, ], }, ] diff --git a/vueManager/src/utils/gridDeleteAction.js b/vueManager/src/utils/gridDeleteAction.js new file mode 100644 index 000000000..9a7ac8622 --- /dev/null +++ b/vueManager/src/utils/gridDeleteAction.js @@ -0,0 +1,39 @@ +/** + * Shared delete-action config for settings grids (ActionsColumn / useActions confirm). + * + * Row handlers must not call confirm.require — confirmation happens once in useActions. + */ +export function gridDeleteAction({ + confirmMessage, + confirmTitle = 'confirm_delete', + confirmAccept = 'delete', +}) { + return { + name: 'delete', + handler: 'delete', + icon: 'pi-trash', + label: 'delete', + severity: 'danger', + confirm: true, + confirmTitle, + confirmMessage, + confirmAccept, + } +} + +export function applyDeleteConfirmDefaults( + actions, + { confirmMessage, confirmTitle = 'confirm_delete', confirmAccept = 'delete' } +) { + return actions.map(action => { + const handler = action.handler || action.name + if (handler !== 'delete') return action + return { + ...action, + confirm: true, + confirmTitle: action.confirmTitle || confirmTitle, + confirmMessage: action.confirmMessage || confirmMessage, + confirmAccept: action.confirmAccept || confirmAccept, + } + }) +} diff --git a/vueManager/src/utils/gridDeleteAction.test.js b/vueManager/src/utils/gridDeleteAction.test.js new file mode 100644 index 000000000..cbb724283 --- /dev/null +++ b/vueManager/src/utils/gridDeleteAction.test.js @@ -0,0 +1,23 @@ +import { describe, expect, it } from 'vitest' + +import { applyDeleteConfirmDefaults, gridDeleteAction } from './gridDeleteAction.js' + +describe('gridDeleteAction', () => { + it('builds confirm delete action', () => { + const action = gridDeleteAction({ confirmMessage: 'delivery_delete_confirm_message' }) + expect(action.handler).toBe('delete') + expect(action.confirm).toBe(true) + expect(action.confirmMessage).toBe('delivery_delete_confirm_message') + expect(action.confirmTitle).toBe('confirm_delete') + }) + + it('applyDeleteConfirmDefaults merges delete action from API config', () => { + const actions = applyDeleteConfirmDefaults( + [{ name: 'edit', handler: 'edit' }, { name: 'delete', handler: 'delete', confirm: false }], + { confirmMessage: 'payment_delete_confirm_message' } + ) + expect(actions[1].confirm).toBe(true) + expect(actions[1].confirmMessage).toBe('payment_delete_confirm_message') + expect(actions[0].confirm).toBeUndefined() + }) +}) diff --git a/vueManager/tests/settingsConfirmGroups.test.js b/vueManager/tests/settingsConfirmGroups.test.js index d9051018f..ff8f0f833 100644 --- a/vueManager/tests/settingsConfirmGroups.test.js +++ b/vueManager/tests/settingsConfirmGroups.test.js @@ -16,6 +16,15 @@ const GRIDS = [ ['LinksGrid.vue', 'settings-links'], ] +/** Grids that confirm row delete only via ActionsColumn / useActions (#630). */ +const ROW_DELETE_VIA_ACTIONS = new Set([ + 'DeliveriesGrid.vue', + 'PaymentsGrid.vue', + 'StatusesGrid.vue', + 'VendorsGrid.vue', + 'LinksGrid.vue', +]) + function read(name) { return fs.readFileSync(path.join(srcRoot, name), 'utf8') } @@ -34,11 +43,13 @@ test('settings tab grids isolate ConfirmDialog with unique groups (#548)', () => assert.match(text, /]*:group="CONFIRM_GROUP"/, `${file} ConfirmDialog must bind :group`) - const requireAt = [...text.matchAll(/confirm\.require\s*\(/g)] - assert.ok(requireAt.length > 0, `${file} must have confirm.require`) - for (const match of requireAt) { - const snippet = text.slice(match.index, match.index + 400) - assert.match(snippet, /\bgroup:\s*CONFIRM_GROUP/, `${file} confirm.require must pass group: CONFIRM_GROUP`) + if (!ROW_DELETE_VIA_ACTIONS.has(file)) { + const requireAt = [...text.matchAll(/confirm\.require\s*\(/g)] + assert.ok(requireAt.length > 0, `${file} must have confirm.require`) + for (const match of requireAt) { + const snippet = text.slice(match.index, match.index + 400) + assert.match(snippet, /\bgroup:\s*CONFIRM_GROUP/, `${file} confirm.require must pass group: CONFIRM_GROUP`) + } } if (text.includes(' { + for (const [file, fnName] of ROW_DELETE_HANDLERS) { + const text = read(file) + const fnRe = new RegExp(`async function ${fnName}[\\s\\S]*?^}`, 'm') + const match = text.match(fnRe) + assert.ok(match, `${file} must define async function ${fnName}`) + assert.ok( + !match[0].includes('confirm.require'), + `${file} ${fnName} must not open a second ConfirmDialog` + ) + assert.match( + text, + /gridDeleteAction\s*\(/, + `${file} must use gridDeleteAction() for row delete confirm (not handler confirm.require)` + ) + } +}) From 96d1372aeacda44dec34c197b8a5c05f7fef5a2c Mon Sep 17 00:00:00 2001 From: Ibochkarev <2138260+Ibochkarev@users.noreply.github.com> Date: Sun, 6 Sep 2026 08:27:30 +0200 Subject: [PATCH 13/19] =?UTF-8?q?PR=20#637:=20fix(web-api):=20CORS=20prefl?= =?UTF-8?q?ight=20OPTIONS=20=D0=B4=D0=BE=20CorsMiddleware?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit https://github.com/modx-pro/MiniShop3/pull/637 --- .../src/Middleware/CorsMiddleware.php | 6 +- .../minishop3/src/Router/Router.php | 38 +++++++ .../HeadlessStorefrontCorsRouterTest.php | 104 ++++++++++++++++++ .../WebApi/HeadlessStorefrontCorsTest.php | 74 +++---------- 4 files changed, 159 insertions(+), 63 deletions(-) create mode 100644 core/components/minishop3/tests/Integration/WebApi/HeadlessStorefrontCorsRouterTest.php diff --git a/core/components/minishop3/src/Middleware/CorsMiddleware.php b/core/components/minishop3/src/Middleware/CorsMiddleware.php index 5df2110a3..d990695f7 100644 --- a/core/components/minishop3/src/Middleware/CorsMiddleware.php +++ b/core/components/minishop3/src/Middleware/CorsMiddleware.php @@ -2,6 +2,7 @@ namespace MiniShop3\Middleware; +use MiniShop3\Router\HttpStatus; use MiniShop3\Router\Middleware\MiddlewareInterface; use MiniShop3\Router\Response; use MiniShop3\Utils\CorsConfig; @@ -56,11 +57,10 @@ public function handle(array $params) $this->setCorsHeaders($origin); } - // For preflight requests (OPTIONS) immediately return 200 + // Preflight: stop middleware chain with 200 (Router/api.php sends the envelope). $method = $_SERVER['REQUEST_METHOD'] ?? 'GET'; if ($method === 'OPTIONS') { - http_response_code(200); - exit; + return Response::success(null, null, HttpStatus::OK); } return null; // Continue execution diff --git a/core/components/minishop3/src/Router/Router.php b/core/components/minishop3/src/Router/Router.php index badc15d61..2b9dfa11c 100644 --- a/core/components/minishop3/src/Router/Router.php +++ b/core/components/minishop3/src/Router/Router.php @@ -381,6 +381,10 @@ public function dispatch(?string $uri = null, ?string $method = null): Response return Response::error('Route not found', HttpStatus::NOT_FOUND); case Dispatcher::METHOD_NOT_ALLOWED: + if ($httpMethod === 'OPTIONS') { + return $this->dispatchOptionsPreflight($uri, $routeInfo[1] ?? []); + } + return Response::error('Method not allowed', HttpStatus::METHOD_NOT_ALLOWED); case Dispatcher::FOUND: @@ -393,6 +397,40 @@ public function dispatch(?string $uri = null, ?string $method = null): Response return Response::error('Unknown error', HttpStatus::INTERNAL_SERVER_ERROR); } + /** + * Run middleware for a valid storefront path when FastRoute has no OPTIONS handler. + * + * Never invokes the route handler: preflight must not trigger POST/GET side effects (#634). + * CorsMiddleware (first on stock /api/v1 routes) returns 200 and stops the chain. + * + * @param list $allowedMethods + */ + protected function dispatchOptionsPreflight(string $uri, array $allowedMethods): Response + { + if (!self::isStorefrontRoute($uri) || $allowedMethods === []) { + return Response::error('Method not allowed', HttpStatus::METHOD_NOT_ALLOWED); + } + + $probeMethod = $allowedMethods[0]; + $probeInfo = $this->dispatcher->dispatch($probeMethod, $uri); + + if ($probeInfo[0] !== Dispatcher::FOUND) { + return Response::error('Method not allowed', HttpStatus::METHOD_NOT_ALLOWED); + } + + $vars = array_merge($_GET, $probeInfo[2] ?? []); + + foreach ($probeInfo[1]['middlewares'] ?? [] as $middleware) { + $result = $this->resolveMiddleware($middleware)->handle($vars); + + if ($result instanceof Response) { + return $result; + } + } + + return Response::error('Method not allowed', HttpStatus::METHOD_NOT_ALLOWED); + } + /** * Execute route handler with middleware * diff --git a/core/components/minishop3/tests/Integration/WebApi/HeadlessStorefrontCorsRouterTest.php b/core/components/minishop3/tests/Integration/WebApi/HeadlessStorefrontCorsRouterTest.php new file mode 100644 index 000000000..e4f5828b4 --- /dev/null +++ b/core/components/minishop3/tests/Integration/WebApi/HeadlessStorefrontCorsRouterTest.php @@ -0,0 +1,104 @@ +dispatch( + 'OPTIONS', + '/api/v1/health', + [], + [], + [ + 'Origin' => 'https://shop.example', + 'Access-Control-Request-Method' => 'POST', + ], + ); + + self::assertSame(200, $res['status']); + self::assertTrue($res['success']); + } + + public function testOptionsPreflightOnPostOnlyCartAddReturns200(): void + { + $res = $this->dispatch( + 'OPTIONS', + '/api/v1/cart/add', + [], + [], + [ + 'Origin' => 'https://shop.example', + 'Access-Control-Request-Method' => 'POST', + ], + ); + + self::assertSame(200, $res['status']); + self::assertTrue($res['success']); + } + + public function testOptionsPreflightOnUnknownStorefrontPathReturns404(): void + { + $res = $this->dispatch( + 'OPTIONS', + '/api/v1/no-such-endpoint', + [], + [], + ['Origin' => 'https://shop.example'], + ); + + self::assertSame(404, $res['status']); + self::assertFalse($res['success']); + } + + public function testOptionsPreflightDisallowedOriginStillReturns200Envelope(): void + { + $res = $this->dispatch( + 'OPTIONS', + '/api/v1/health', + [], + [], + ['Origin' => 'https://evil.example'], + ); + + self::assertSame(200, $res['status']); + self::assertTrue($res['success']); + } + + public function testOptionsPreflightDoesNotInvokeHandlerWithoutCorsMiddleware(): void + { + $handlerCalled = false; + $router = new \MiniShop3\Router\Router($this->modx); + $router->group('/api/v1', function ($router) use (&$handlerCalled) { + $router->post('/probe-mutation', function () use (&$handlerCalled) { + $handlerCalled = true; + + return \MiniShop3\Router\Response::success(['mutated' => true]); + }); + }, []); + $router->build(); + + $_SERVER['REQUEST_METHOD'] = 'OPTIONS'; + $_SERVER['REQUEST_URI'] = '/api/v1/probe-mutation'; + $_REQUEST = ['route' => '/api/v1/probe-mutation']; + + $response = $router->dispatch('/api/v1/probe-mutation', 'OPTIONS'); + + self::assertFalse($handlerCalled); + self::assertSame(405, $response->getStatusCode()); + } + + public function testGetHealthStillWorksAfterCorsChanges(): void + { + $res = $this->dispatch('GET', '/api/v1/health'); + + self::assertSame(200, $res['status']); + self::assertTrue($res['success']); + } +} diff --git a/core/components/minishop3/tests/Integration/WebApi/HeadlessStorefrontCorsTest.php b/core/components/minishop3/tests/Integration/WebApi/HeadlessStorefrontCorsTest.php index c2bc3c6ca..a23f1607b 100644 --- a/core/components/minishop3/tests/Integration/WebApi/HeadlessStorefrontCorsTest.php +++ b/core/components/minishop3/tests/Integration/WebApi/HeadlessStorefrontCorsTest.php @@ -5,13 +5,13 @@ namespace MiniShop3\Tests\Integration\WebApi; use MiniShop3\Middleware\CorsMiddleware; +use MiniShop3\Router\HttpStatus; +use MiniShop3\Router\Response; use MiniShop3\Utils\CorsConfig; use PHPUnit\Framework\TestCase; /** - * CORS smoke adjacent to journey suite (#574 / #335). - * - * Full Router OPTIONS cannot run in-process: CorsMiddleware exits after 200. + * CORS smoke adjacent to journey suite (#574 / #335, #634). */ final class HeadlessStorefrontCorsTest extends TestCase { @@ -26,67 +26,21 @@ public function testNormalizeRejectsWildcardWithCredentials(): void self::assertFalse($cfg['allow_credentials']); } - public function testPreflightOptionsExitsWithHttp200ForAllowlistedOrigin(): void + public function testPreflightOptionsReturnsHttp200Response(): void { - $script = <<<'PHP' - 'https://shop.example', - 'allow_credentials' => true, -]); -$originOk = CorsConfig::isOriginAllowed('https://shop.example', ['https://shop.example']) ? '1' : '0'; - -register_shutdown_function(static function () use ($originOk): void { - echo 'HTTP_CODE=' . (string) http_response_code() . "\n"; - echo 'ORIGIN_OK=' . $originOk . "\n"; -}); - -$mw->handle([]); -fwrite(STDERR, "OPTIONS did not exit\n"); -exit(2); -PHP; - - $autoload = dirname(__DIR__, 3) . '/vendor/autoload.php'; - - $tmp = tempnam(sys_get_temp_dir(), 'ms3-cors-'); - self::assertNotFalse($tmp); - file_put_contents($tmp, $script); + $_SERVER['REQUEST_METHOD'] = 'OPTIONS'; + $_SERVER['HTTP_ORIGIN'] = 'https://shop.example'; - $cmd = sprintf( - '%s %s %s', - escapeshellarg(PHP_BINARY), - escapeshellarg($tmp), - escapeshellarg($autoload) - ); + $mw = new CorsMiddleware([ + 'allowed_origins' => 'https://shop.example', + 'allow_credentials' => true, + ]); - $descriptors = [ - 0 => ['pipe', 'r'], - 1 => ['pipe', 'w'], - 2 => ['pipe', 'w'], - ]; - $proc = proc_open($cmd, $descriptors, $pipes, dirname($tmp)); - self::assertIsResource($proc); - fclose($pipes[0]); - $stdout = stream_get_contents($pipes[1]); - fclose($pipes[1]); - $stderr = stream_get_contents($pipes[2]); - fclose($pipes[2]); - $code = proc_close($proc); - @unlink($tmp); + $result = $mw->handle([]); - self::assertSame(0, $code, $stderr); - self::assertStringContainsString('HTTP_CODE=200', (string) $stdout); - self::assertStringContainsString('ORIGIN_OK=1', (string) $stdout); - self::assertStringNotContainsString('OPTIONS did not exit', (string) $stderr); + self::assertInstanceOf(Response::class, $result); + self::assertSame(HttpStatus::OK, $result->getStatusCode()); + self::assertTrue($result->getData()['success'] ?? false); } public function testNormalizeEmptyOriginsDisallowsAll(): void From 229dbf0c0e832b64e0e14cd3dbd2e8045c615105 Mon Sep 17 00:00:00 2001 From: Ibochkarev <2138260+Ibochkarev@users.noreply.github.com> Date: Sun, 6 Sep 2026 08:27:30 +0200 Subject: [PATCH 14/19] PR #639: feat(web-api): prefill order draft from customer profile after auth https://github.com/modx-pro/MiniShop3/pull/639 --- .../src/Services/Customer/AuthManager.php | 38 ++++++ .../Services/Order/OrderAddressManager.php | 30 +++++ .../src/Services/Order/OrderDraftManager.php | 31 +++-- .../Customer/AuthManagerLifecycleTest.php | 34 +++++- .../OrderAddressManagerProfilePrefillTest.php | 115 ++++++++++++++++++ 5 files changed, 235 insertions(+), 13 deletions(-) create mode 100644 core/components/minishop3/tests/Unit/Services/Order/OrderAddressManagerProfilePrefillTest.php diff --git a/core/components/minishop3/src/Services/Customer/AuthManager.php b/core/components/minishop3/src/Services/Customer/AuthManager.php index f18892e22..d1f1a57d2 100644 --- a/core/components/minishop3/src/Services/Customer/AuthManager.php +++ b/core/components/minishop3/src/Services/Customer/AuthManager.php @@ -6,6 +6,8 @@ use MiniShop3\Controllers\Auth\PasswordAuthProvider; use MiniShop3\Model\msCustomer; use MiniShop3\Model\msCustomerToken; +use MiniShop3\Services\Cart\CartDraftContext; +use MiniShop3\Services\Order\OrderAddressManager; use MiniShop3\Services\Order\OrderDraftManager; use MiniShop3\Services\TokenService; use MiniShop3\Utils\CookieHelper; @@ -256,12 +258,48 @@ public function establishCustomerSession(msCustomer $customer): ?array session_regenerate_id(true); } + $this->prefillOrderDraftFromCustomer($customer, $tokenString, $draftManager); + return [ 'token' => $tokenString, 'expires_at' => $tokenObj->get('expires_at'), ]; } + /** + * Copy empty profile fields from the authenticated customer into their draft order. + * + * Failures are logged and must not block login/register. + */ + private function prefillOrderDraftFromCustomer( + msCustomer $customer, + string $token, + OrderDraftManager $draftManager + ): void { + if ($token === '' || !$this->modx->services->has('ms3_order_address_manager')) { + return; + } + + try { + $pageCtx = $this->modx->context->key ?? CartDraftContext::DEFAULT_CONTEXT; + $ctx = CartDraftContext::resolve($this->modx, $pageCtx); + $draft = $draftManager->findDraftByToken($token, $ctx); + if (!$draft) { + return; + } + + /** @var OrderAddressManager $addressManager */ + $addressManager = $this->modx->services->get('ms3_order_address_manager'); + $addressManager->prefillProfileFieldsFromCustomer($draft, $customer); + } catch (\Throwable $e) { + $this->modx->log( + modX::LOG_LEVEL_ERROR, + '[AuthManager] prefillOrderDraftFromCustomer failed for customer #' + . (int) $customer->get('id') . ': ' . $e->getMessage() + ); + } + } + /** * Bind API token after verified identity (email verify, etc.). * diff --git a/core/components/minishop3/src/Services/Order/OrderAddressManager.php b/core/components/minishop3/src/Services/Order/OrderAddressManager.php index d8edbaca1..65edc66e6 100644 --- a/core/components/minishop3/src/Services/Order/OrderAddressManager.php +++ b/core/components/minishop3/src/Services/Order/OrderAddressManager.php @@ -3,8 +3,10 @@ namespace MiniShop3\Services\Order; use MiniShop3\MiniShop3; +use MiniShop3\Model\msCustomer; use MiniShop3\Model\msCustomerAddress; use MiniShop3\Model\msOrder; +use MiniShop3\Services\Customer\CustomerPublicDto; use MODX\Revolution\modX; /** @@ -185,6 +187,34 @@ public function fillFromCustomer(msOrder $draft, array &$orderData, array $custo } } + /** + * Prefill empty checkout profile fields on the customer's draft from their public profile. + * + * Only fills fields that are still empty in the draft (does not overwrite checkout edits). + */ + public function prefillProfileFieldsFromCustomer(msOrder $draft, msCustomer $customer): void + { + $draftCustomerId = (int) $draft->get('customer_id'); + $customerId = (int) $customer->get('id'); + if ($draftCustomerId <= 0 || $draftCustomerId !== $customerId) { + return; + } + + $customerData = []; + foreach (CustomerPublicDto::CORE_PROFILE_EDITABLE_FIELDS as $field) { + $value = $customer->get($field); + if ($value !== null && $value !== '') { + $customerData[$field] = $value; + } + } + if ($customerData === []) { + return; + } + + $orderData = $this->draftManager->toArray($draft); + $this->fillFromCustomer($draft, $orderData, $customerData); + } + /** * Shorthand for success response */ diff --git a/core/components/minishop3/src/Services/Order/OrderDraftManager.php b/core/components/minishop3/src/Services/Order/OrderDraftManager.php index 84fe13a94..00f945062 100644 --- a/core/components/minishop3/src/Services/Order/OrderDraftManager.php +++ b/core/components/minishop3/src/Services/Order/OrderDraftManager.php @@ -29,6 +29,24 @@ public function __construct(modX $modx, MiniShop3 $ms3) $this->ms3 = $ms3; } + /** + * Find draft order by session token only (no customer_id fallback). + */ + public function findDraftByToken(string $token, string $ctx = 'web'): ?msOrder + { + if ($token === '') { + return null; + } + + $statusDraft = (int) $this->modx->getOption('ms3_status_draft', null, 1) ?: 1; + + return $this->modx->getObject(msOrder::class, [ + 'token' => $token, + 'status_id' => $statusDraft, + 'context' => $ctx, + ]); + } + /** * Get existing draft order by token * @@ -43,27 +61,20 @@ public function getDraft(string $token, string $ctx = 'web'): ?msOrder return null; } - $status_draft = (int) $this->modx->getOption('ms3_status_draft', null, 1) ?: 1; - - // 1. Try to find by token first (primary method) - $draft = $this->modx->getObject(msOrder::class, [ - 'token' => $token, - 'status_id' => $status_draft, - 'context' => $ctx, - ]); - + $draft = $this->findDraftByToken($token, $ctx); if ($draft) { return $draft; } // 2. Fallback: search by customer_id for authenticated customers // Sort by id DESC to get the most recent draft when multiple exist + $statusDraft = (int) $this->modx->getOption('ms3_status_draft', null, 1) ?: 1; $customerId = (int)($_SESSION['ms3']['customer_id'] ?? 0); if ($customerId > 0) { $q = $this->modx->newQuery(msOrder::class); $q->where([ 'customer_id' => $customerId, - 'status_id' => $status_draft, + 'status_id' => $statusDraft, 'context' => $ctx, ]); $q->sortby('id', 'DESC'); diff --git a/core/components/minishop3/tests/Integration/Customer/AuthManagerLifecycleTest.php b/core/components/minishop3/tests/Integration/Customer/AuthManagerLifecycleTest.php index 77079d275..c4d1be6f2 100644 --- a/core/components/minishop3/tests/Integration/Customer/AuthManagerLifecycleTest.php +++ b/core/components/minishop3/tests/Integration/Customer/AuthManagerLifecycleTest.php @@ -6,7 +6,9 @@ use MiniShop3\Model\msCustomer; use MiniShop3\Model\msCustomerToken; +use MiniShop3\Model\msOrder; use MiniShop3\Services\Customer\AuthManager; +use MiniShop3\Services\Order\OrderAddressManager; use MiniShop3\Services\Order\OrderDraftManager; use MiniShop3\Services\TokenService; use MiniShop3\Tests\Support\CustomerAuthPdoStore; @@ -27,6 +29,8 @@ class AuthManagerLifecycleTest extends TestCase /** @var list */ private array $draftCalls = []; + private int $prefillCalls = 0; + protected function setUp(): void { if (!class_exists(modX::class, false)) { @@ -49,6 +53,7 @@ protected function setUp(): void $this->store = $this->createStore(); $this->store->reset(); $this->draftCalls = []; + $this->prefillCalls = 0; } protected function tearDown(): void @@ -128,6 +133,7 @@ public function testEstablishSessionRotatesTokenAndLogoutMintsGuest(): void self::assertNull($this->store->findToken(['token' => $guestToken, 'type' => msCustomerToken::TYPE_API])); self::assertSame(1, $this->store->countTokens((int) $customer->id, msCustomerToken::TYPE_API)); self::assertContains('transfer:' . $guestToken . '=>' . $loginToken, $this->draftCalls); + self::assertSame(1, $this->prefillCalls); self::assertTrue($auth->logoutCurrentCustomer()); self::assertSame(0, (int) ($_SESSION['ms3']['customer_id'] ?? 0)); @@ -319,6 +325,12 @@ private function makeModx(array $options = []): modX { $store = $this->store; $draftCalls = &$this->draftCalls; + $prefillCalls = &$this->prefillCalls; + + $draft = $this->createStub(msOrder::class); + $draft->method('get')->willReturnMap([ + ['customer_id', 1], + ]); $orderDraftManager = $this->createStub(OrderDraftManager::class); $orderDraftManager->method('transferDraftToToken')->willReturnCallback( @@ -335,8 +347,17 @@ static function (string $token, int $customerId, string $ctx = 'web') use (&$dra return true; } ); + $orderDraftManager->method('getDraft')->willReturn($draft); + $orderDraftManager->method('findDraftByToken')->willReturn($draft); + + $addressManager = $this->createStub(OrderAddressManager::class); + $addressManager->method('prefillProfileFieldsFromCustomer')->willReturnCallback( + static function () use (&$prefillCalls): void { + $prefillCalls++; + } + ); - $modx = new class ($store, $options, $orderDraftManager) extends modX { + $modx = new class ($store, $options, $orderDraftManager, $addressManager) extends modX { /** * @param array $options */ @@ -344,19 +365,25 @@ public function __construct( private CustomerAuthPdoStore $store, private array $options, OrderDraftManager $orderDraftManager, + OrderAddressManager $addressManager, ) { parent::__construct(); $tokenService = new TokenService($this); - $this->services = new class ($tokenService, $orderDraftManager) { + $this->services = new class ($tokenService, $orderDraftManager, $addressManager) { public function __construct( private TokenService $tokenService, private OrderDraftManager $orderDraftManager, + private OrderAddressManager $addressManager, ) { } public function has(string $key): bool { - return in_array($key, ['ms3_token_service', 'ms3_order_draft_manager'], true); + return in_array($key, [ + 'ms3_token_service', + 'ms3_order_draft_manager', + 'ms3_order_address_manager', + ], true); } public function get(string $key): mixed @@ -364,6 +391,7 @@ public function get(string $key): mixed return match ($key) { 'ms3_token_service' => $this->tokenService, 'ms3_order_draft_manager' => $this->orderDraftManager, + 'ms3_order_address_manager' => $this->addressManager, default => null, }; } diff --git a/core/components/minishop3/tests/Unit/Services/Order/OrderAddressManagerProfilePrefillTest.php b/core/components/minishop3/tests/Unit/Services/Order/OrderAddressManagerProfilePrefillTest.php new file mode 100644 index 000000000..be18e3925 --- /dev/null +++ b/core/components/minishop3/tests/Unit/Services/Order/OrderAddressManagerProfilePrefillTest.php @@ -0,0 +1,115 @@ +makeManager(); + $draft = $this->createStub(msOrder::class); + $orderData = [ + 'address_first_name' => 'Guest', + 'address_last_name' => '', + 'address_email' => '', + 'address_phone' => '', + ]; + + $manager->fillFromCustomer($draft, $orderData, [ + 'first_name' => 'Jane', + 'last_name' => 'Doe', + 'email' => 'jane@example.com', + 'phone' => '+79990001122', + ]); + + self::assertSame('Guest', $orderData['address_first_name']); + self::assertSame('Doe', $orderData['address_last_name']); + self::assertSame('jane@example.com', $orderData['address_email']); + self::assertSame('+79990001122', $orderData['address_phone']); + } + + public function testPrefillProfileFieldsFromCustomerRefusesForeignDraft(): void + { + $fieldManager = $this->createMock(OrderFieldManager::class); + $fieldManager->expects(self::never())->method('add'); + + $manager = $this->makeManager($fieldManager); + $draft = $this->createStub(msOrder::class); + $draft->method('get')->willReturnMap([ + ['customer_id', 99], + ]); + $customer = $this->createStub(msCustomer::class); + $customer->method('get')->willReturnMap([ + ['id', 42], + ['first_name', 'Jane'], + ['last_name', 'Doe'], + ['email', 'jane@example.com'], + ]); + + $manager->prefillProfileFieldsFromCustomer($draft, $customer); + } + + public function testPrefillProfileFieldsFromCustomerFillsEmptyDraftFields(): void + { + $fieldManager = $this->createMock(OrderFieldManager::class); + $fieldManager->expects(self::exactly(3)) + ->method('add') + ->willReturn(['success' => true, 'message' => '', 'data' => []]); + + $manager = $this->makeManager($fieldManager); + $draft = $this->createStub(msOrder::class); + $draft->method('get')->willReturnMap([ + ['customer_id', 42], + ]); + $customer = $this->createStub(msCustomer::class); + $customer->method('get')->willReturnMap([ + ['id', 42], + ['first_name', 'Jane'], + ['last_name', 'Doe'], + ['email', 'jane@example.com'], + ['password', 'must-not-leak'], + ]); + + $manager->prefillProfileFieldsFromCustomer($draft, $customer); + } + + private function makeManager(?OrderFieldManager $fieldManager = null): OrderAddressManager + { + $modx = $this->createStub(modX::class); + $ms3 = $this->createStub(MiniShop3::class); + + $draftManager = $this->createStub(OrderDraftManager::class); + $draftManager->method('toArray')->willReturn([ + 'customer_id' => 42, + 'address_first_name' => '', + 'address_last_name' => '', + 'address_email' => '', + 'address_phone' => '', + ]); + + $fieldManager ??= $this->createStub(OrderFieldManager::class); + if (!($fieldManager instanceof \PHPUnit\Framework\MockObject\MockObject)) { + $fieldManager->method('add')->willReturn(['success' => true, 'message' => '', 'data' => []]); + } + + return new OrderAddressManager($modx, $ms3, $draftManager, $fieldManager); + } +} From f99f60b60c0c924e450fec661c6652bbf6865da1 Mon Sep 17 00:00:00 2001 From: Ibochkarev <2138260+Ibochkarev@users.noreply.github.com> Date: Sun, 6 Sep 2026 08:27:30 +0200 Subject: [PATCH 15/19] PR #642: fix(vue): nested category checks survive reload on product Categories tab https://github.com/modx-pro/MiniShop3/pull/642 --- .../components/ResourceCategoryTree.test.js | 222 ++++++++++++++++++ .../src/components/ResourceCategoryTree.vue | 78 ++++-- vueManager/src/test/stubs/useLexicon.js | 3 + vueManager/vitest.config.js | 3 + 4 files changed, 286 insertions(+), 20 deletions(-) create mode 100644 vueManager/src/components/ResourceCategoryTree.test.js create mode 100644 vueManager/src/test/stubs/useLexicon.js diff --git a/vueManager/src/components/ResourceCategoryTree.test.js b/vueManager/src/components/ResourceCategoryTree.test.js new file mode 100644 index 000000000..d8782021b --- /dev/null +++ b/vueManager/src/components/ResourceCategoryTree.test.js @@ -0,0 +1,222 @@ +/* eslint-disable vue/one-component-per-file -- PrimeVue stubs for unit tests */ +import { flushPromises, mount } from '@vue/test-utils' +import { afterEach, describe, expect, it, vi } from 'vitest' +import { defineComponent, h, ref } from 'vue' + +import request from '../request.js' +import ResourceCategoryTree from './ResourceCategoryTree.vue' + +vi.mock('../request.js', () => ({ + default: { + get: vi.fn(), + }, +})) + +const TreeStub = defineComponent({ + name: 'TreeStub', + props: { value: { type: Array, default: () => [] } }, + setup(props, { slots }) { + return () => + h( + 'div', + { class: 'tree-stub' }, + (props.value || []).map(node => slots.default?.({ node })) + ) + }, +}) + +const CheckboxStub = defineComponent({ + name: 'CheckboxStub', + props: { + modelValue: { type: Boolean, default: false }, + disabled: { type: Boolean, default: false }, + inputId: { type: String, default: '' }, + }, + emits: ['update:modelValue'], + setup(props, { emit }) { + return () => + h('input', { + type: 'checkbox', + class: 'tree-checkbox-stub', + checked: props.modelValue, + disabled: props.disabled, + id: props.inputId, + onChange: event => emit('update:modelValue', event.target.checked), + }) + }, +}) + +const ContextMenuStub = defineComponent({ + name: 'ContextMenu', + setup(_, { expose }) { + expose({ show: vi.fn() }) + return () => h('div', { class: 'context-menu-stub' }) + }, +}) + +const globalStubs = { + Tree: TreeStub, + Checkbox: CheckboxStub, + ContextMenu: ContextMenuStub, +} + +function categoryRow(id, overrides = {}) { + return { + id, + label: `Category ${id}`, + leaf: true, + checked: false, + selectable: true, + locked: false, + class_key: 'MiniShop3\\Model\\msCategory', + published: 1, + hidemenu: 0, + ...overrides, + } +} + +function mockCategoryRows(...rows) { + request.get.mockResolvedValue({ results: rows }) +} + +function mountTree(options = {}) { + const { + modelValue = [], + lockedIds = [], + apiUrl = '/api/mgr/product-data/1/categories/tree', + apiParams = {}, + onUpdateModelValue, + } = options + + const selected = ref([...modelValue]) + const emitLog = [] + + // Close the real v-model loop (emit → props update → watch) so #546 recursion is detectable. + const Host = defineComponent({ + setup() { + return () => + h(ResourceCategoryTree, { + modelValue: selected.value, + apiUrl, + apiParams, + lockedIds, + inputIdPrefix: 'test-cat-', + 'onUpdate:modelValue': value => { + selected.value = [...value] + emitLog.push([...value]) + onUpdateModelValue?.(value) + }, + }) + }, + }) + + const wrapper = mount(Host, { + global: { + stubs: globalStubs, + }, + }) + + return { wrapper, selected, emitLog } +} + +describe('ResourceCategoryTree', () => { + afterEach(() => { + vi.clearAllMocks() + }) + + it('keeps nestedDeep in selection after loadRoot when root page omits it (#641)', async () => { + const parentId = 10 + const nestedDeepId = 999 + + mockCategoryRows(categoryRow(parentId, { checked: true, locked: true })) + + const { selected, emitLog } = mountTree({ + modelValue: [parentId, nestedDeepId], + lockedIds: [parentId], + }) + + await flushPromises() + + expect(selected.value).toEqual(expect.arrayContaining([parentId, nestedDeepId])) + expect(selected.value).toHaveLength(2) + for (const ids of emitLog) { + expect(ids).toContain(nestedDeepId) + } + }) + + it('removes a visible category from modelValue when unchecked', async () => { + const removableId = 20 + + mockCategoryRows(categoryRow(removableId, { checked: true })) + + const { wrapper, selected } = mountTree({ + modelValue: [removableId], + }) + + await flushPromises() + + const checkbox = wrapper.find(`#test-cat-${removableId}`) + expect(checkbox.exists()).toBe(true) + expect(checkbox.element.checked).toBe(true) + + await checkbox.setValue(false) + await flushPromises() + + expect(selected.value).not.toContain(removableId) + expect(selected.value).toHaveLength(0) + }) + + it('enforces locked ids and emits when parent omitted them', async () => { + const lockedId = 10 + + mockCategoryRows(categoryRow(lockedId, { checked: true, locked: true })) + + const { selected, emitLog } = mountTree({ + modelValue: [], + lockedIds: [lockedId], + }) + + await flushPromises() + + expect(selected.value).toEqual([lockedId]) + expect(emitLog.length).toBeGreaterThan(0) + expect(emitLog.at(-1)).toEqual([lockedId]) + }) + + it('does not emit recursively when modelValue already includes locked ids (#546)', async () => { + const lockedId = 10 + const nestedDeepId = 999 + + mockCategoryRows(categoryRow(lockedId, { checked: true, locked: true })) + + const { emitLog } = mountTree({ + modelValue: [lockedId, nestedDeepId], + lockedIds: [lockedId], + }) + + await flushPromises() + + expect(emitLog.length).toBeLessThanOrEqual(1) + }) + + it('cannot uncheck a locked category', async () => { + const lockedId = 10 + + mockCategoryRows(categoryRow(lockedId, { checked: true, locked: true })) + + const { wrapper, selected } = mountTree({ + modelValue: [lockedId], + lockedIds: [lockedId], + }) + + await flushPromises() + + const checkbox = wrapper.find(`#test-cat-${lockedId}`) + expect(checkbox.element.disabled).toBe(true) + + await checkbox.setValue(false) + await flushPromises() + + expect(selected.value).toEqual([lockedId]) + }) +}) diff --git a/vueManager/src/components/ResourceCategoryTree.vue b/vueManager/src/components/ResourceCategoryTree.vue index bfc679403..bea6603c4 100644 --- a/vueManager/src/components/ResourceCategoryTree.vue +++ b/vueManager/src/components/ResourceCategoryTree.vue @@ -34,7 +34,15 @@ const contextMenu = ref(null) const contextNode = ref(null) const checkedSet = ref(new Set()) -const lockedSet = computed(() => new Set(props.lockedIds.map(id => Number(id)))) +/** @returns {number|null} Positive category id, or null if invalid. */ +function toCategoryId(value) { + const id = Number(value) + return Number.isFinite(id) && id > 0 ? id : null +} + +const lockedSet = computed( + () => new Set(props.lockedIds.map(toCategoryId).filter(id => id !== null)) +) const contextMenuItems = computed(() => [ { @@ -88,48 +96,73 @@ function toTreeNode(row) { data: { class_key: row.class_key, selectable: row.selectable !== false, - locked: isApiRowLocked(row), + locked: isLockedId(row.id, row.locked), published: row.published, hidemenu: row.hidemenu, }, } } +function normalizeIdList(ids) { + if (!Array.isArray(ids)) { + return [] + } + return ids.map(toCategoryId).filter(id => id !== null) +} + +function nodeId(node) { + return toCategoryId(node?.id) +} + +function isLockedId(id, flaggedLocked = false) { + const normalized = toCategoryId(id) + return Boolean(flaggedLocked) || (normalized !== null && lockedSet.value.has(normalized)) +} + +function applyLockedIds(base) { + const next = new Set(base) + for (const id of lockedSet.value) { + next.add(id) + } + return next +} + function mergeCheckedFromApi(rows) { const next = new Set(checkedSet.value) for (const row of rows) { - if (row.checked || lockedSet.value.has(Number(row.id))) { - next.add(row.id) + const id = toCategoryId(row.id) + if (id !== null && (row.checked || isLockedId(id, row.locked))) { + next.add(id) } } checkedSet.value = next } -function isApiRowLocked(row) { - return Boolean(row.locked) || lockedSet.value.has(Number(row.id)) -} - function isSelectableNode(node) { return !!node?.data?.selectable } function isLockedNode(node) { - return !!node?.data?.locked || lockedSet.value.has(Number(node?.id)) + return isLockedId(node?.id, node?.data?.locked) } function isChecked(node) { - return checkedSet.value.has(node.id) + return checkedSet.value.has(nodeId(node)) } function toggleNode(node, checked) { if (!isSelectableNode(node) || (isLockedNode(node) && !checked)) { return } + const id = nodeId(node) + if (id === null) { + return + } const next = new Set(checkedSet.value) if (checked) { - next.add(node.id) + next.add(id) } else { - next.delete(node.id) + next.delete(id) } checkedSet.value = next emitSelection() @@ -146,8 +179,10 @@ async function loadRoot() { } function ensureLockedChecked() { - const next = new Set(checkedSet.value) - lockedSet.value.forEach(id => next.add(id)) + const next = applyLockedIds(checkedSet.value) + if (next.size === checkedSet.value.size) { + return + } checkedSet.value = next emitSelection() } @@ -211,10 +246,14 @@ async function bulkToggleChecks(node, checked) { const next = new Set(checkedSet.value) function walk(n) { if (isSelectableNode(n)) { + const id = nodeId(n) + if (id === null) { + return + } if (checked || isLockedNode(n)) { - next.add(n.id) + next.add(id) } else { - next.delete(n.id) + next.delete(id) } } if (Array.isArray(n.children)) { @@ -255,15 +294,14 @@ watch( // Calling emitSelection() unconditionally here echoes the value we just received, // which reassigns props.modelValue and retriggers this watch — an infinite recursive // loop that freezes the page (#546). - const incoming = new Set(newIds || []) - const next = new Set(incoming) - lockedSet.value.forEach(id => next.add(id)) + const incoming = new Set(normalizeIdList(newIds)) + const next = applyLockedIds(incoming) checkedSet.value = next if (next.size !== incoming.size) { emitSelection() } }, - { deep: true } + { deep: true, immediate: true } ) watch(lockedSet, () => ensureLockedChecked()) diff --git a/vueManager/src/test/stubs/useLexicon.js b/vueManager/src/test/stubs/useLexicon.js new file mode 100644 index 000000000..ea0223007 --- /dev/null +++ b/vueManager/src/test/stubs/useLexicon.js @@ -0,0 +1,3 @@ +export function useLexicon() { + return { _: key => key } +} diff --git a/vueManager/vitest.config.js b/vueManager/vitest.config.js index ec1c05dfe..9aa5b23f9 100644 --- a/vueManager/vitest.config.js +++ b/vueManager/vitest.config.js @@ -8,6 +8,9 @@ export default defineConfig({ resolve: { alias: { '@': fileURLToPath(new URL('./src', import.meta.url)), + '@vuetools/useLexicon': fileURLToPath( + new URL('./src/test/stubs/useLexicon.js', import.meta.url) + ), }, }, test: { From 624f5557ebdc75cf606ba3422b356b8fc6884134 Mon Sep 17 00:00:00 2001 From: Ibochkarev <2138260+Ibochkarev@users.noreply.github.com> Date: Sun, 6 Sep 2026 08:27:30 +0200 Subject: [PATCH 16/19] =?UTF-8?q?PR=20#643:=20=D0=93=D0=B0=D0=BB=D0=B5?= =?UTF-8?q?=D1=80=D0=B5=D1=8F:=20=D1=81=D0=BE=D1=80=D1=82=D0=B8=D1=80?= =?UTF-8?q?=D0=BE=D0=B2=D0=BA=D0=B0=20=D0=BF=D0=BE=20=D0=B8=D0=BC=D0=B5?= =?UTF-8?q?=D0=BD=D0=B8=20=D0=BF=D0=BE=D1=81=D0=BB=D0=B5=20=D0=BF=D0=B0?= =?UTF-8?q?=D0=BA=D0=B5=D1=82=D0=BD=D0=BE=D0=B9=20=D0=B7=D0=B0=D0=B3=D1=80?= =?UTF-8?q?=D1=83=D0=B7=D0=BA=D0=B8?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit https://github.com/modx-pro/MiniShop3/pull/643 --- .../src/Processors/Gallery/SortByName.php | 56 +++++++++++++ .../Services/Product/ProductImageService.php | 78 ++++++++++++++++++ .../ProductImageServiceSortByNameTest.php | 81 +++++++++++++++++++ .../src/components/gallery/ProductGallery.vue | 37 ++++++++- vueManager/src/composables/useGalleryApi.js | 13 +++ 5 files changed, 262 insertions(+), 3 deletions(-) create mode 100644 core/components/minishop3/src/Processors/Gallery/SortByName.php create mode 100644 core/components/minishop3/tests/Unit/Services/Product/ProductImageServiceSortByNameTest.php diff --git a/core/components/minishop3/src/Processors/Gallery/SortByName.php b/core/components/minishop3/src/Processors/Gallery/SortByName.php new file mode 100644 index 000000000..aeaef46bc --- /dev/null +++ b/core/components/minishop3/src/Processors/Gallery/SortByName.php @@ -0,0 +1,56 @@ +getProperty('product_id'); + + if ($productId <= 0) { + return $this->failure($this->modx->lexicon('ms3_gallery_err_ns')); + } + + /** @var msProductData|null $productData */ + $productData = $this->modx->getObject(msProductData::class, ['id' => $productId]); + if (!$productData) { + return $this->failure($this->modx->lexicon('ms3_gallery_err_no_product')); + } + + /** @var ProductImageService|null $imageService */ + $imageService = $this->modx->services->get('ms3_product_image'); + if (!$imageService instanceof ProductImageService) { + return $this->failure($this->modx->lexicon('ms3_err_unknown')); + } + + $saved = $imageService->sortProductImagesByName($productData); + if ($saved === false) { + return $this->failure($this->modx->lexicon('ms3_err_unknown')); + } + + $thumb = (string) $productData->get('thumb'); + if ($thumb === '') { + /** @var MiniShop3 $ms3 */ + $ms3 = $this->modx->services->get('ms3'); + $thumb = (string) ($ms3->config['defaultThumb'] ?? ''); + } + + return $this->success('', [ + 'thumb' => $thumb, + ]); + } +} diff --git a/core/components/minishop3/src/Services/Product/ProductImageService.php b/core/components/minishop3/src/Services/Product/ProductImageService.php index ee89b52a1..ce48347a0 100644 --- a/core/components/minishop3/src/Services/Product/ProductImageService.php +++ b/core/components/minishop3/src/Services/Product/ProductImageService.php @@ -119,6 +119,74 @@ public function rankProductImages(msProductData $productData, array $ranks): boo return true; } + /** + * Build natural-sort positions for gallery rows by name (fallback: file), then id. + * + * @param list $rows + * @return array file_id => position (0..n-1) + */ + public static function buildNaturalSortRanks(array $rows): array + { + if ($rows === []) { + return []; + } + + usort($rows, static function (array $a, array $b): int { + $cmp = strnatcasecmp(self::naturalSortKey($a), self::naturalSortKey($b)); + if ($cmp !== 0) { + return $cmp; + } + + $cmp = strnatcasecmp((string) ($a['file'] ?? ''), (string) ($b['file'] ?? '')); + if ($cmp !== 0) { + return $cmp; + } + + return (int) ($a['id'] ?? 0) <=> (int) ($b['id'] ?? 0); + }); + + $ranks = []; + foreach ($rows as $position => $row) { + $ranks[(int) $row['id']] = $position; + } + + return $ranks; + } + + /** + * Re-rank top-level gallery files by natural sort on name/file (#616). + * + * Includes all parent_id=0 rows (not only type=image), matching Upload position counting. + * + * @return bool|mixed save result from updateProductImage(), or true when gallery is empty + */ + public function sortProductImagesByName(msProductData $productData): mixed + { + $productId = (int) $productData->get('id'); + + $rows = []; + + /** @var msProductFile $file */ + foreach ($this->modx->getIterator(msProductFile::class, [ + 'product_id' => $productId, + 'parent_id' => 0, + ]) as $file) { + $rows[] = [ + 'id' => (int) $file->get('id'), + 'name' => (string) $file->get('name'), + 'file' => (string) $file->get('file'), + ]; + } + + if ($rows === []) { + return true; + } + + $this->rankProductImages($productData, self::buildNaturalSortRanks($rows)); + + return $this->updateProductImage($productData); + } + /** * Set which gallery file is the product preview without changing sort order (#130). * @@ -233,6 +301,16 @@ private function getMainGalleryFile(int $productId, ?int $fileId = null): ?msPro return $file ?: null; } + /** + * @param array{name?: string, file?: string} $row + */ + private static function naturalSortKey(array $row): string + { + $name = trim((string) ($row['name'] ?? '')); + + return $name !== '' ? $name : (string) ($row['file'] ?? ''); + } + /** * Remove empty product catalog * diff --git a/core/components/minishop3/tests/Unit/Services/Product/ProductImageServiceSortByNameTest.php b/core/components/minishop3/tests/Unit/Services/Product/ProductImageServiceSortByNameTest.php new file mode 100644 index 000000000..e100982a0 --- /dev/null +++ b/core/components/minishop3/tests/Unit/Services/Product/ProductImageServiceSortByNameTest.php @@ -0,0 +1,81 @@ + 3, 'name' => '10.jpg', 'file' => '10.jpg'], + ['id' => 1, 'name' => '2.jpg', 'file' => '2.jpg'], + ['id' => 2, 'name' => '01.jpg', 'file' => '01.jpg'], + ]; + + $ranks = ProductImageService::buildNaturalSortRanks($rows); + + self::assertSame([2 => 0, 1 => 1, 3 => 2], $ranks); + } + + /** + * Upload processor stores name without extension (01.jpg → "01"). + */ + public function testBuildNaturalSortRanksOrdersExtensionlessUploadNames(): void + { + $rows = [ + ['id' => 3, 'name' => '10', 'file' => 'hash10.jpg'], + ['id' => 1, 'name' => '2', 'file' => 'hash2.jpg'], + ['id' => 2, 'name' => '01', 'file' => 'hash01.jpg'], + ]; + + $ranks = ProductImageService::buildNaturalSortRanks($rows); + + self::assertSame([2 => 0, 1 => 1, 3 => 2], $ranks); + } + + public function testBuildNaturalSortRanksIsCaseInsensitive(): void + { + $rows = [ + ['id' => 1, 'name' => 'B.jpg', 'file' => 'b.jpg'], + ['id' => 2, 'name' => 'a.jpg', 'file' => 'a.jpg'], + ]; + + $ranks = ProductImageService::buildNaturalSortRanks($rows); + + self::assertSame([2 => 0, 1 => 1], $ranks); + } + + public function testBuildNaturalSortRanksFallsBackToFileWhenNameEmpty(): void + { + $rows = [ + ['id' => 1, 'name' => '', 'file' => 'z.jpg'], + ['id' => 2, 'name' => '', 'file' => 'a.jpg'], + ]; + + $ranks = ProductImageService::buildNaturalSortRanks($rows); + + self::assertSame([2 => 0, 1 => 1], $ranks); + } + + public function testBuildNaturalSortRanksTieBreaksById(): void + { + $rows = [ + ['id' => 5, 'name' => 'same.jpg', 'file' => 'same.jpg'], + ['id' => 2, 'name' => 'same.jpg', 'file' => 'same.jpg'], + ]; + + $ranks = ProductImageService::buildNaturalSortRanks($rows); + + self::assertSame([2 => 0, 5 => 1], $ranks); + } + + public function testBuildNaturalSortRanksReturnsEmptyForEmptyInput(): void + { + self::assertSame([], ProductImageService::buildNaturalSortRanks([])); + } +} diff --git a/vueManager/src/components/gallery/ProductGallery.vue b/vueManager/src/components/gallery/ProductGallery.vue index 3ea3e67f3..d09baeaec 100644 --- a/vueManager/src/components/gallery/ProductGallery.vue +++ b/vueManager/src/components/gallery/ProductGallery.vue @@ -37,6 +37,7 @@ const { isLoading, fetchGalleryList, sortFiles, + sortFilesByName, deleteFiles, deleteAll, regenerateThumbs, @@ -346,10 +347,40 @@ function onChangeSource(sourceId) { } /** - * Handle upload events + * Serialize overlapping Uppy `complete` handlers (allowMultipleUploadBatches) + * so SortByName does not race on the same product in one tab (#616). */ -function onUploadComplete() { - loadImages() +let uploadCompleteQueue = Promise.resolve() + +/** + * After batch upload: natural-sort positions by filename (#616), then reload grid. + */ +function onUploadComplete(result) { + uploadCompleteQueue = uploadCompleteQueue + .then(() => runUploadComplete(result)) + .catch(() => {}) +} + +async function runUploadComplete(result) { + const hasUploads = (result?.successful?.length ?? 0) > 0 + + try { + if (hasUploads) { + const { thumb } = await sortFilesByName(props.productId) + if (thumb) { + updateProductThumb(thumb) + } + } + } catch (error) { + toast.add({ + severity: 'error', + summary: _('ms3_gallery_errors'), + detail: error.message, + life: 5000, + }) + } finally { + await loadImages() + } } onMounted(() => { diff --git a/vueManager/src/composables/useGalleryApi.js b/vueManager/src/composables/useGalleryApi.js index ba4805967..cbf0fcc93 100644 --- a/vueManager/src/composables/useGalleryApi.js +++ b/vueManager/src/composables/useGalleryApi.js @@ -131,6 +131,18 @@ export function useGalleryApi() { return { thumb: data.object?.thumb || '' } } + /** + * Sort gallery files by natural filename order (#616). + * @param {number} productId + * @returns {Promise<{thumb: string}>} + */ + async function sortFilesByName(productId) { + const data = await connectorRequest('MiniShop3\\Processors\\Gallery\\SortByName', { + product_id: productId, + }) + return { thumb: data.object?.thumb || '' } + } + /** * Delete files by IDs * @param {number[]} ids @@ -226,6 +238,7 @@ export function useGalleryApi() { isLoading, fetchGalleryList, sortFiles, + sortFilesByName, deleteFiles, deleteAll, regenerateThumbs, From f645f1a1899d1302a5afbdb93baca4b9e417372a Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Sun, 6 Sep 2026 08:27:30 +0200 Subject: [PATCH 17/19] PR #647: chore(deps): bump postcss-selector-parser in /vueManager https://github.com/modx-pro/MiniShop3/pull/647 --- vueManager/package-lock.json | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/vueManager/package-lock.json b/vueManager/package-lock.json index ea0a92eb0..a218f3ab2 100644 --- a/vueManager/package-lock.json +++ b/vueManager/package-lock.json @@ -5414,9 +5414,9 @@ } }, "node_modules/postcss-selector-parser": { - "version": "6.1.2", - "resolved": "https://registry.npmjs.org/postcss-selector-parser/-/postcss-selector-parser-6.1.2.tgz", - "integrity": "sha512-Q8qQfPiZ+THO/3ZrOrO0cJJKfpYCagtMUkXbnEfmgUjwXg6z/WBeOyS9APBBPCTSiDV+s4SwQGu8yFsiMRIudg==", + "version": "6.1.4", + "resolved": "https://registry.npmjs.org/postcss-selector-parser/-/postcss-selector-parser-6.1.4.tgz", + "integrity": "sha512-bIoJLOmjCO1S9XdY/DcnR5hJxvrDir1PbGChrzXG3vw0/FOliy/fA3dmdhQ441kah4gKv+TwckGzex6wNS5cnQ==", "dev": true, "license": "MIT", "dependencies": { @@ -6593,9 +6593,9 @@ "license": "CC0-1.0" }, "node_modules/stylelint-scss/node_modules/postcss-selector-parser": { - "version": "7.1.1", - "resolved": "https://registry.npmjs.org/postcss-selector-parser/-/postcss-selector-parser-7.1.1.tgz", - "integrity": "sha512-orRsuYpJVw8LdAwqqLykBj9ecS5/cRHlI5+nvTo8LcCKmzDmqVORXtOIYEEQuL9D4BxtA1lm5isAqzQZCoQ6Eg==", + "version": "7.1.5", + "resolved": "https://registry.npmjs.org/postcss-selector-parser/-/postcss-selector-parser-7.1.5.tgz", + "integrity": "sha512-KvvtD7SrlBP7dlgkBghEE3r84CABm5SmV2aNcG4oCA+qDnJ/tvKonFVvwWAyyWUEwxuNawdfEAZKP9zM3oZ2Uw==", "dev": true, "license": "MIT", "dependencies": { @@ -6708,9 +6708,9 @@ } }, "node_modules/stylelint/node_modules/postcss-selector-parser": { - "version": "7.1.1", - "resolved": "https://registry.npmjs.org/postcss-selector-parser/-/postcss-selector-parser-7.1.1.tgz", - "integrity": "sha512-orRsuYpJVw8LdAwqqLykBj9ecS5/cRHlI5+nvTo8LcCKmzDmqVORXtOIYEEQuL9D4BxtA1lm5isAqzQZCoQ6Eg==", + "version": "7.1.5", + "resolved": "https://registry.npmjs.org/postcss-selector-parser/-/postcss-selector-parser-7.1.5.tgz", + "integrity": "sha512-KvvtD7SrlBP7dlgkBghEE3r84CABm5SmV2aNcG4oCA+qDnJ/tvKonFVvwWAyyWUEwxuNawdfEAZKP9zM3oZ2Uw==", "dev": true, "license": "MIT", "dependencies": { From a45919abe9136b7f3e3cc8ef5f4e3e1a9f5d57dd Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Sun, 6 Sep 2026 08:27:30 +0200 Subject: [PATCH 18/19] PR #648: chore(deps-dev): bump @humanfs/node from 0.16.6 to 0.16.8 in /vueManager https://github.com/modx-pro/MiniShop3/pull/648 --- vueManager/package-lock.json | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/vueManager/package-lock.json b/vueManager/package-lock.json index a218f3ab2..041247307 100644 --- a/vueManager/package-lock.json +++ b/vueManager/package-lock.json @@ -1274,41 +1274,41 @@ } }, "node_modules/@humanfs/core": { - "version": "0.19.1", - "resolved": "https://registry.npmjs.org/@humanfs/core/-/core-0.19.1.tgz", - "integrity": "sha512-5DyQ4+1JEUzejeK1JGICcideyfUbGixgS9jNgex5nqkW+cY7WZhxBigmieN5Qnw9ZosSNVC9KQKyb+GUaGyKUA==", + "version": "0.19.2", + "resolved": "https://registry.npmjs.org/@humanfs/core/-/core-0.19.2.tgz", + "integrity": "sha512-UhXNm+CFMWcbChXywFwkmhqjs3PRCmcSa/hfBgLIb7oQ5HNb1wS0icWsGtSAUNgefHeI+eBrA8I1fxmbHsGdvA==", "dev": true, "license": "Apache-2.0", + "dependencies": { + "@humanfs/types": "^0.15.0" + }, "engines": { "node": ">=18.18.0" } }, "node_modules/@humanfs/node": { - "version": "0.16.6", - "resolved": "https://registry.npmjs.org/@humanfs/node/-/node-0.16.6.tgz", - "integrity": "sha512-YuI2ZHQL78Q5HbhDiBA1X4LmYdXCKCMQIfw0pw7piHJwyREFebJUvrQN4cMssyES6x+vfUbx1CIpaQUKYdQZOw==", + "version": "0.16.8", + "resolved": "https://registry.npmjs.org/@humanfs/node/-/node-0.16.8.tgz", + "integrity": "sha512-gE1eQNZ3R++kTzFUpdGlpmy8kDZD/MLyHqDwqjkVQI0JMdI1D51sy1H958PNXYkM2rAac7e5/CnIKZrHtPh3BQ==", "dev": true, "license": "Apache-2.0", "dependencies": { - "@humanfs/core": "^0.19.1", - "@humanwhocodes/retry": "^0.3.0" + "@humanfs/core": "^0.19.2", + "@humanfs/types": "^0.15.0", + "@humanwhocodes/retry": "^0.4.0" }, "engines": { "node": ">=18.18.0" } }, - "node_modules/@humanfs/node/node_modules/@humanwhocodes/retry": { - "version": "0.3.1", - "resolved": "https://registry.npmjs.org/@humanwhocodes/retry/-/retry-0.3.1.tgz", - "integrity": "sha512-JBxkERygn7Bv/GbN5Rv8Ul6LVknS+5Bp6RgDC/O8gEBU/yeH5Ui5C/OlWrTb6qct7LjjfT6Re2NxB0ln0yYybA==", + "node_modules/@humanfs/types": { + "version": "0.15.0", + "resolved": "https://registry.npmjs.org/@humanfs/types/-/types-0.15.0.tgz", + "integrity": "sha512-ZZ1w0aoQkwuUuC7Yf+7sdeaNfqQiiLcSRbfI08oAxqLtpXQr9AIVX7Ay7HLDuiLYAaFPu8oBYNq/QIi9URHJ3Q==", "dev": true, "license": "Apache-2.0", "engines": { - "node": ">=18.18" - }, - "funding": { - "type": "github", - "url": "https://github.com/sponsors/nzakas" + "node": ">=18.18.0" } }, "node_modules/@humanwhocodes/module-importer": { From 4e4c37cbfde72f30a26238c318cf45b70734e6d8 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Sun, 6 Sep 2026 08:27:30 +0200 Subject: [PATCH 19/19] PR #649: chore(deps): bump fast-uri from 3.1.5 to 3.1.7 in /vueManager https://github.com/modx-pro/MiniShop3/pull/649 --- vueManager/package-lock.json | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/vueManager/package-lock.json b/vueManager/package-lock.json index 041247307..e30b639c2 100644 --- a/vueManager/package-lock.json +++ b/vueManager/package-lock.json @@ -3733,9 +3733,9 @@ "license": "MIT" }, "node_modules/fast-uri": { - "version": "3.1.5", - "resolved": "https://registry.npmjs.org/fast-uri/-/fast-uri-3.1.5.tgz", - "integrity": "sha512-gHwA1O9LDIcKunMKhObS/HimwtehO1nPUECKAu5TpKgaO19fcWEl4bliWe1jWxVFvIXztJjjQ4L8XQ1EU9f7Jw==", + "version": "3.1.7", + "resolved": "https://registry.npmjs.org/fast-uri/-/fast-uri-3.1.7.tgz", + "integrity": "sha512-dOvZVzjdZdz7phd9v6jCbwxrBW3fK6n8Rc0CtdmM4bumzMnxywBYhuph6J819RRw/ku+rLbelwfMunktuzVVHg==", "dev": true, "funding": [ {