Migrate "Orders > Shopping Carts" page #35007

boherm · 2024-01-09T13:19:23Z

Questions	Answers
Branch?	develop
Description?	"Orders > Carts" page migration
Type?	improvement
Category?	BO
BC breaks?	no
Deprecations?	no
How to test?	Before, please build admin new-theme with `make admin-new-theme` ! First, active `Carts index` feature flag, and then navigate into Orders -> Carts page. You can filter data, delete not already ordered carts one by one or by bulk actions. Multistore mode is supported, and in that case we add Shop column into grid (only for all / group context).
Fixed ticket?	Fixes #10584
Related PRs	#31934
Sponsor company

Warning:
For now new Kpi use legacy to retrieve data in all already migrated pages!

prestonBot · 2024-01-09T13:19:26Z

Hello @boherm!

This is your first pull request on the PrestaShop project. Thank you, and welcome to this Open Source community!

M0rgan01 · 2024-01-15T08:48:05Z

src/Adapter/Cart/CommandHandler/BulkDeleteCartHandler.php

+    /**
+     * @var CartRepository
+     */
+    private $cartRepository;
+
+    /**
+     * @var OrderRepository
+     */
+    private $orderRepository;
+
+    /**
+     * @param CartRepository $cartRepository
+     */
+    public function __construct(CartRepository $cartRepository, OrderRepository $orderRepository)
+    {
+        $this->cartRepository = $cartRepository;
+        $this->orderRepository = $orderRepository;
+    }


We could type, or even use the construct to define class variables

Yep you can use property promotion now, which probably wasn't possible when you started the PR, but I'd say it can be optional if you have time to do it Maybe not blocking

M0rgan01 · 2024-01-15T08:48:31Z

src/Adapter/Cart/CommandHandler/DeleteCartHandler.php

+    /**
+     * @var CartRepository
+     */
+    private $cartRepository;
+
+    /**
+     * @var OrderRepository
+     */
+    private $orderRepository;
+
+    /**
+     * @param CartRepository $cartRepository
+     */
+    public function __construct(CartRepository $cartRepository, OrderRepository $orderRepository)
+    {
+        $this->cartRepository = $cartRepository;
+        $this->orderRepository = $orderRepository;
+    }


We could type, or even use the construct to define class variables

M0rgan01 · 2024-01-15T08:52:51Z

src/Adapter/Cart/CommandHandler/DeleteCartHandler.php

+    public function handle(DeleteCartCommand $command): void
+    {
+        $order = $this->orderRepository->findByCartId($command->getCartId());
+        if ($order) {
+            throw new CannotDeleteOrderedCartException(sprintf('Cart "%s" with order cannot be deleted.', $command->getCartId()->getValue()));
+        }
+        $this->cartRepository->delete($command->getCartId());
+    }


This part is practically the same as the bulk delete, without the loop. Maybe this can be merged

I would advise to keep them separated ;)

If you forget they look similar, do you think it makes sense the code for bulk delete and code for delete is the same? Aren't they two different things?

I think the implementations coincide but it's just random

https://github.com/PrestaShop/PrestaShop/blob/develop/src/Adapter/Profile/CommandHandler/BulkDeleteProfileHandler.php
https://github.com/PrestaShop/PrestaShop/blob/develop/src/Adapter/Profile/CommandHandler/DeleteProfileHandler.php

Both handlers do the same thing so it's normal they look alike, however each command implementation should remain independent to avoid too much coupling In this case three lines of code that are duplicates are acceptable I think Maintaining a common trait or abstract class would probably cost us more in the long run

M0rgan01 · 2024-01-15T08:53:34Z

src/Adapter/Kpi/AbandonedCartKpi.php

+    /**
+     * @var TranslatorInterface
+     */
+    private $translator;
+
+    /**
+     * @var ConfigurationInterface
+     */
+    private $configuration;
+
+    /**
+     * @var LegacyContext
+     */
+    private $contextAdapter;
+
+    /**
+     * @var UrlGeneratorInterface
+     */
+    private $router;
+
+    /**
+     * @var FeatureFlagRepository
+     */
+    private $featureFlag;
+
+    /**
+     * @param TranslatorInterface $translator
+     * @param ConfigurationInterface $configuration
+     * @param LegacyContext $contextAdapter
+     * @param UrlGeneratorInterface $router
+     * @param FeatureFlagRepository $featureFlag
+     */
+    public function __construct(
+        TranslatorInterface $translator,
+        ConfigurationInterface $configuration,
+        LegacyContext $contextAdapter,
+        UrlGeneratorInterface $router,
+        FeatureFlagRepository $featureFlag
+    ) {
+        $this->translator = $translator;
+        $this->configuration = $configuration;
+        $this->contextAdapter = $contextAdapter;
+        $this->router = $router;
+        $this->featureFlag = $featureFlag;
+    }


We could type, or even use the construct to define class variables

M0rgan01 · 2024-01-15T08:54:51Z

src/Adapter/Kpi/AbandonedCartKpi.php

+        $helper->href = $this->contextAdapter->getAdminLink('AdminCarts', true, [
+            'action' => 'filterOnlyAbandonedCarts',
+        ]);
+
+        if ($this->featureFlag->isEnabled(FeatureFlagSettings::FEATURE_FLAG_CARTS)) {
+            $helper->href = $this->router->generate('admin_carts_index', [
+                'cart[filters][status]' => CartStatus::ABANDONED_CART,
+            ]);
+        }


Suggested change

$helper->href = $this->contextAdapter->getAdminLink('AdminCarts', true, [

'action' => 'filterOnlyAbandonedCarts',

]);

if ($this->featureFlag->isEnabled(FeatureFlagSettings::FEATURE_FLAG_CARTS)) {

$helper->href = $this->router->generate('admin_carts_index', [

'cart[filters][status]' => CartStatus::ABANDONED_CART,

]);

}

if ($this->featureFlag->isEnabled(FeatureFlagSettings::FEATURE_FLAG_CARTS)) {

$helper->href = $this->router->generate('admin_carts_index', [

'cart[filters][status]' => CartStatus::ABANDONED_CART,

]);

} else {

$helper->href = $this->contextAdapter->getAdminLink('AdminCarts', true, [

'action' => 'filterOnlyAbandonedCarts',

]);

}

M0rgan01 · 2024-01-15T09:16:45Z

src/PrestaShopBundle/Resources/config/services/core/grid/doctrine_query_builder.yml

@@ -536,3 +536,11 @@ services:
      $languageId: '@=service("prestashop.adapter.legacy.context").getContext().language.id'
      $connection: '@doctrine.dbal.default_connection'
      $dbPrefix: '%database_prefix%'
+
+  prestashop.core.grid.query.builder.cart:


Use FQCN instead

M0rgan01 · 2024-01-15T09:17:07Z

src/PrestaShopBundle/Resources/config/services/core/grid/grid_data_factory.yml

+      - '@prestashop.core.grid.query.doctrine_query_parser'
+      - 'cart'
+
+  prestashop.core.grid.data.factory.cart_decorator:


Use FQCN instead

M0rgan01 · 2024-01-15T09:17:13Z

src/PrestaShopBundle/Resources/config/services/core/grid/grid_definition_factory.yml

@@ -428,3 +428,14 @@ services:
    calls:
      - [ setTranslator, [ '@translator' ] ]
    deprecated: ~
+
+  prestashop.core.grid.definition.factory.cart:


Use FQCN instead

M0rgan01 · 2024-01-15T09:18:13Z

src/PrestaShopBundle/Resources/config/services/core/grid/grid_definition_factory.yml

+      - '@=service("prestashop.adapter.legacy.context").getContext().language.date_format_full'
+      - '@prestashop.adapter.shop.context'
+      - '@=service("prestashop.adapter.multistore_feature").isActive()'


it is better to do these operations in the class

src/PrestaShopBundle/Resources/config/services/core/kpi.yml

matks

Great work 😉

I provided little things to improve/fix but it's very minor

matks · 2024-01-15T10:03:53Z

admin-dev/themes/new-theme/js/pages/cart/index.ts

@@ -0,0 +1,51 @@
+/**
+ * vCopyright since 2007 PrestaShop SA and Contributors


Suggested change

* vCopyright since 2007 PrestaShop SA and Contributors

* Copyright since 2007 PrestaShop SA and Contributors

Little typo, I guess a copy-paste missclick 😄

matks · 2024-01-15T10:10:40Z

src/Adapter/Cart/CommandHandler/DeleteCartHandler.php

+    public function handle(DeleteCartCommand $command): void
+    {
+        $order = $this->orderRepository->findByCartId($command->getCartId());
+        if ($order) {
+            throw new CannotDeleteOrderedCartException(sprintf('Cart "%s" with order cannot be deleted.', $command->getCartId()->getValue()));
+        }
+        $this->cartRepository->delete($command->getCartId());
+    }


I would advise to keep them separated ;)

If you forget they look similar, do you think it makes sense the code for bulk delete and code for delete is the same? Aren't they two different things?

I think the implementations coincide but it's just random

https://github.com/PrestaShop/PrestaShop/blob/develop/src/Adapter/Profile/CommandHandler/BulkDeleteProfileHandler.php
https://github.com/PrestaShop/PrestaShop/blob/develop/src/Adapter/Profile/CommandHandler/DeleteProfileHandler.php

matks · 2024-01-15T10:12:10Z

src/Adapter/Kpi/AbandonedCartKpi.php

    /**
     * {@inheritdoc}
     */
    public function render()
    {
-        $translator = Context::getContext()->getTranslator();
+        $dateFormat = $this->contextAdapter->getLanguage()->date_format_lite;


Now that we have the modern contexts you should prefer using them so you can inject LanguageContext instead here, date_format_lite is synonym to LanguageContext->getDateFormat()

src/PrestaShopBundle/Controller/Admin/Sell/Order/CartController.php

jolelievre

Thanks @boherm

A few comments here and there but it's looking almost finished, the suggestions from Morgan about property promotion would be a nice bonus but I won't block only for that personally

jolelievre · 2024-01-15T11:39:03Z

src/Adapter/Cart/CommandHandler/BulkDeleteCartHandler.php

+    /**
+     * @var CartRepository
+     */
+    private $cartRepository;
+
+    /**
+     * @var OrderRepository
+     */
+    private $orderRepository;
+
+    /**
+     * @param CartRepository $cartRepository
+     */
+    public function __construct(CartRepository $cartRepository, OrderRepository $orderRepository)
+    {
+        $this->cartRepository = $cartRepository;
+        $this->orderRepository = $orderRepository;
+    }


Yep you can use property promotion now, which probably wasn't possible when you started the PR, but I'd say it can be optional if you have time to do it Maybe not blocking

jolelievre · 2024-01-15T11:47:53Z

src/Adapter/Cart/CommandHandler/BulkDeleteCartHandler.php

+ * Deletes cart in bulk action using legacy object model
+ */
+#[AsCommandHandler]
+final class BulkDeleteCartHandler implements BulkDeleteCartHandlerInterface


You couldn't know about it but a class as introduced for bulk handlers AbstractBulkCommandHandler there is an example of usage here:

PrestaShop/src/Adapter/Alias/CommandHandler/BulkDeleteAliasHandler.php

Line 45 in 525b8da

class BulkDeleteAliasHandler extends AbstractBulkCommandHandler implements BulkDeleteAliasHandlerInterface

It has two advantages:

it mutualizes the code to handle bulk commands which is almost always the same except for the specific part that performs the single action

it forces using an exception that implements BulkCommandExceptionInterface this interface allows handling an exception for each entity that failed thus allowing more accurate error message in the UX

jolelievre · 2024-01-15T11:53:36Z

src/Adapter/Cart/CommandHandler/DeleteCartHandler.php

+    public function handle(DeleteCartCommand $command): void
+    {
+        $order = $this->orderRepository->findByCartId($command->getCartId());
+        if ($order) {
+            throw new CannotDeleteOrderedCartException(sprintf('Cart "%s" with order cannot be deleted.', $command->getCartId()->getValue()));
+        }
+        $this->cartRepository->delete($command->getCartId());
+    }


Both handlers do the same thing so it's normal they look alike, however each command implementation should remain independent to avoid too much coupling In this case three lines of code that are duplicates are acceptable I think Maintaining a common trait or abstract class would probably cost us more in the long run

jolelievre · 2024-01-15T11:55:47Z

src/Adapter/Order/Repository/OrderRepository.php

+     *
+     * @throws CoreException
+     */
+    public function findByCartId(CartId $cartId): ?Order


Suggested change

public function findByCartId(CartId $cartId): ?Order

public function getByCartId(CartId $cartId): Order

To be more consistent with the usual get method in the object model repositories Also, instead of returning null maybe you should throw an OrderNotFoundException, also to be more consistent with the get behaviour

Either that or you could implement a public function getOrderIdByCartId(CartId $cartId): ?OrderId method (this one could have a nullable return), which would match more with the usage you have (check if an order is associated to the cart) and it prevents fetching the whole Order object which you don't need in your delete handlers

jolelievre · 2024-01-15T12:00:39Z

src/Adapter/Kpi/AbandonedCartKpi.php

    /**
     * {@inheritdoc}
     */
    public function render()
    {
-        $translator = Context::getContext()->getTranslator();
+        $dateFormat = $this->contextAdapter->getLanguage()->date_format_lite;


Now that we have the modern contexts you should prefer using them so you can inject LanguageContext instead here, date_format_lite is synonym to LanguageContext->getDateFormat()

jolelievre · 2024-01-15T13:33:53Z

src/PrestaShopBundle/Resources/config/routing/admin/sell/orders/carts.yml

+    _controller: PrestaShopBundle\Controller\Admin\Sell\Order\CartController::indexAction
+    _legacy_controller: AdminCarts
+    _legacy_link: AdminCarts
+    _legacy_feature_flag: carts


It's good to add the feature flag on the new routes, but you probably should also add it on the existing ones in this file

jolelievre · 2024-01-15T13:34:41Z

src/PrestaShopBundle/Resources/config/services/adapter/cart.yml

+  prestashop.adapter.cart.command_handler.delete_cart_handler:
+    class: 'PrestaShop\PrestaShop\Adapter\Cart\CommandHandler\DeleteCartHandler'


Suggested change

prestashop.adapter.cart.command_handler.delete_cart_handler:

class: 'PrestaShop\PrestaShop\Adapter\Cart\CommandHandler\DeleteCartHandler'

PrestaShop\PrestaShop\Adapter\Cart\CommandHandler\DeleteCartHandler:

We favor the FQCN notation now, even for private services and handlers

jolelievre · 2024-01-15T13:34:55Z

src/PrestaShopBundle/Resources/config/services/adapter/cart.yml

+  prestashop.adapter.cart.command_handler.bulk_delete_cart_handler:
+    class: 'PrestaShop\PrestaShop\Adapter\Cart\CommandHandler\BulkDeleteCartHandler'


Suggested change

prestashop.adapter.cart.command_handler.bulk_delete_cart_handler:

class: 'PrestaShop\PrestaShop\Adapter\Cart\CommandHandler\BulkDeleteCartHandler'

PrestaShop\PrestaShop\Adapter\Cart\CommandHandler\BulkDeleteCartHandler:

jolelievre · 2024-01-15T13:38:43Z

src/Core/Grid/Definition/Factory/CartGridDefinitionFactory.php

+     */
+    public function __construct(
+        HookDispatcherInterface $hookDispatcher,
+        string $contextDateFormat,


You should inject LanguageContext instead

jolelievre · 2024-01-15T13:42:59Z

...Integration/Behaviour/Features/Scenario/Cart/CartRule/BackOffice/delete_cart_from_bo.feature

+    Then cart "dummy_cart1" should exist because cart is already ordered
+    And cart "dummy_cart2" should exist
+    And cart "dummy_cart3" should exist


This actually shows an issue here, the bulk action shouldn't stop because one of the actions failed It's supposed to keep doing the delete on the whole list and store if any of them failed

But this comment is actually in line with my previous suggestions on the bulk handler, the suggested abstract class is actually here to help implementing this kind of behavior and we'd like to make it generic on all bulk actions for consistency

- Add alignement system in column - Add `badge_type_field` to use column in grid to select badge type directly and no only color (like `color_field` for instance)

- Use DI instead of `Context` - Add purple color - Create kpi_row factory for Carts page kpi

- Add index action page - Add solo/bulk delete actions - Add export into csv

Add cart JS grid query grid

fix repository

- Add new column for display & search status - Fix AbandonedCartKpi to go directly into abandoned cart fix grid improvement

boherm · 2024-02-09T08:39:39Z

Some tests before merging: https://github.com/boherm/ga.tests.ui.pr/actions/runs/7841541053

boherm requested a review from a team as a code owner January 9, 2024 13:19

boherm added Improvement Type: Improvement develop Branch labels Jan 9, 2024

matks added the migration symfony migration project label Jan 15, 2024

M0rgan01 reviewed Jan 15, 2024

View reviewed changes

matks reviewed Jan 15, 2024

View reviewed changes

jolelievre reviewed Jan 15, 2024

View reviewed changes

boherm added 13 commits February 7, 2024 17:40

Add new feature flag to enable/disable new Carts page in bo

03688ba

Improve BadgeColumn for Grids

4ba05f3

- Add alignement system in column - Add `badge_type_field` to use column in grid to select badge type directly and no only color (like `color_field` for instance)

Improve used KPI

abe8d69

- Use DI instead of `Context` - Add purple color - Create kpi_row factory for Carts page kpi

Update CartController

c97b0f2

- Add index action page - Add solo/bulk delete actions - Add export into csv

Add grid and query builder for Carts page

bb02ea8

Add cart JS grid query grid

Add CQRS command to solo/bulk delete carts

37d033d

Fix kpi injections

7835663

Fix delete actions and CartRepository

1562740

fix repository

Move private function in CartGridDefinitionFactory

58dddb8

Improve carts grid

f5309e9

- Add new column for display & search status - Fix AbandonedCartKpi to go directly into abandoned cart fix grid improvement

Add behat tests for deleting carts

932dc22

Apply suggestions from code review

bc09904

Fix after rebase

c4f68ae

boherm force-pushed the #10584-migrate-bo-orders-shopping-carts-page branch from 6e3e69b to c4f68ae Compare February 7, 2024 17:16

Fix after new comments

236e4c7

boherm force-pushed the #10584-migrate-bo-orders-shopping-carts-page branch from 0965d59 to 236e4c7 Compare February 7, 2024 19:05

matks approved these changes Feb 8, 2024

View reviewed changes

M0rgan01 approved these changes Feb 8, 2024

View reviewed changes

boherm added the Waiting for author Status: action required, waiting for author feedback label Feb 9, 2024

WahbiPS removed the Waiting for author Status: action required, waiting for author feedback label Feb 12, 2024

boherm merged commit ff4c55d into PrestaShop:develop Feb 12, 2024
23 checks passed

boherm deleted the #10584-migrate-bo-orders-shopping-carts-page branch February 12, 2024 08:38

boherm added this to the 9.0.0 milestone Feb 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate "Orders > Shopping Carts" page #35007

Migrate "Orders > Shopping Carts" page #35007

boherm commented Jan 9, 2024

prestonBot commented Jan 9, 2024

M0rgan01 Jan 15, 2024

jolelievre Jan 15, 2024

M0rgan01 Jan 15, 2024

M0rgan01 Jan 15, 2024

matks Jan 15, 2024

jolelievre Jan 15, 2024

M0rgan01 Jan 15, 2024

M0rgan01 Jan 15, 2024

M0rgan01 Jan 15, 2024

M0rgan01 Jan 15, 2024

M0rgan01 Jan 15, 2024

M0rgan01 Jan 15, 2024

matks left a comment

matks Jan 15, 2024

matks Jan 15, 2024

matks Jan 15, 2024

jolelievre Jan 15, 2024

jolelievre left a comment

jolelievre Jan 15, 2024

jolelievre Jan 15, 2024

jolelievre Jan 15, 2024

jolelievre Jan 15, 2024

jolelievre Jan 15, 2024

jolelievre Jan 15, 2024

jolelievre Jan 15, 2024

jolelievre Jan 15, 2024

jolelievre Jan 15, 2024

jolelievre Jan 15, 2024

jolelievre Jan 15, 2024

boherm commented Feb 9, 2024

		@@ -0,0 +1,51 @@
		/**
		* vCopyright since 2007 PrestaShop SA and Contributors

	* vCopyright since 2007 PrestaShop SA and Contributors
	* Copyright since 2007 PrestaShop SA and Contributors

	public function findByCartId(CartId $cartId): ?Order
	public function getByCartId(CartId $cartId): Order

		prestashop.adapter.cart.command_handler.delete_cart_handler:
		class: 'PrestaShop\PrestaShop\Adapter\Cart\CommandHandler\DeleteCartHandler'

	prestashop.adapter.cart.command_handler.delete_cart_handler:
	class: 'PrestaShop\PrestaShop\Adapter\Cart\CommandHandler\DeleteCartHandler'
	PrestaShop\PrestaShop\Adapter\Cart\CommandHandler\DeleteCartHandler:

Migrate "Orders > Shopping Carts" page #35007

Migrate "Orders > Shopping Carts" page #35007

Conversation

boherm commented Jan 9, 2024

prestonBot commented Jan 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jolelievre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boherm commented Feb 9, 2024