Customer service controller migration (grid part) #29732

margud · 2022-09-23T12:50:28Z

Questions	Answers
Branch?	develop
Description?	customer service migration. Grid part
Type?	refacto
Category?	BO
BC breaks?	no
Fixed ticket?	Fixes #16592
How to test?	opening sell/customer-service/customer-threads in bo you will see new customer service grid. Grid, view and deletion should already work. Also filter and pagination is added.
Image

src/PrestaShopBundle/Resources/config/routing/admin/sell/customer_service/customer_threads.yml

jolelievre

Thanks @margud

Great (and fast!) work! Just a few things to improve, when the PR is approved I think I'll perform a quick QA by dev and negotiate with QA team so that we can merge it and only ask them to test it when it'll be fully finished.

jolelievre · 2022-10-19T09:08:51Z

admin-dev/themes/new-theme/scss/pages/customer_thread/_customer_thread_main.scss

@@ -0,0 +1,6 @@
+#customer_thread_grid {


Not sure it's worth splitting this into two files considering the amount of code 😅
But it's a detail!

jolelievre · 2022-10-19T09:11:07Z

src/Adapter/CustomerService/CommandHandler/BulkDeleteCustomerThreadHandler.php

+            }
+
+            if (!$customerThread->delete()) {
+                throw new CustomerThreadNotFoundException(sprintf('Cannot delete customer thread with id "%d"', $customerThreadId->getValue()), CustomerThreadNotFoundException::FAILED_DELETE);


I'm not sure the exception is adapted in this case, it should rather be a CannotDeleteCustomerThreadException the entity does exist since you already checked this beforehand So if the deletion fails it's because of another problem, so another type of exception

jolelievre · 2022-10-19T09:11:35Z

src/Adapter/CustomerService/CommandHandler/DeleteCustomerThreadHandler.php

+        }
+
+        if (!$customerThread->delete()) {
+            throw new CustomerThreadNotFoundException(sprintf('Cannot delete customer thread with id "%d"', $command->getCustomerThreadId()->getValue()), CustomerThreadNotFoundException::FAILED_DELETE);


Same suggestion as the bulk handler

jolelievre · 2022-10-19T09:17:44Z

src/Adapter/CustomerService/CommandHandler/DeleteCustomerThreadHandler.php

+        if (!$customerThread->delete()) {
+            throw new CustomerThreadNotFoundException(sprintf('Cannot delete customer thread with id "%d"', $command->getCustomerThreadId()->getValue()), CustomerThreadNotFoundException::FAILED_DELETE);
+        }
+    }


Also now that you are more used to CQRS and migrating pages, I think you could implement this by relying on our repository classes. You'd need to create a CustomerThreadRepository that extends AbstractObjectModelRepository it provides a few helper methods that will help you create the methods you need.

Having a dedicated repository removes the intelligence from the handler into dedicated services that can be used in other context and it prevents duplicating code, it also adds more consistency since all the usual methods are made the same.

Here is an example of implementation:

PrestaShop/src/Adapter/Product/Customization/Repository/CustomizationFieldRepository.php

Line 44 in 5c51e4c

class CustomizationFieldRepository extends AbstractObjectModelRepository

jolelievre · 2022-10-19T09:20:09Z

src/Adapter/CustomerService/CommandHandler/BulkDeleteCustomerThreadHandler.php

+use PrestaShop\PrestaShop\Core\Domain\CustomerService\CommandHandler\BulkDeleteCustomerThreadHandlerInterface;
+use PrestaShop\PrestaShop\Core\Domain\CustomerService\Exception\CustomerThreadNotFoundException;
+
+class BulkDeleteCustomerThreadHandler implements BulkDeleteCustomerThreadHandlerInterface


You should always describe the classes you create, even if it's a short description it gives more context to developers who are not necessarily used to the PrestaShop architecture (contributors, new developers on the project, ...). Since PrestaShop is a CMS/framework it's important to document the code.

For handlers which are based on legacy object usually we describe like this:

Suggested change

class BulkDeleteCustomerThreadHandler implements BulkDeleteCustomerThreadHandlerInterface

/**

* Handles command for customer thread bulk deletion using legacy object model

*/

class BulkDeleteCustomerThreadHandler implements BulkDeleteCustomerThreadHandlerInterface

jolelievre · 2022-10-19T09:48:39Z

src/Core/Grid/Query/CustomerThreadQueryBuilder.php

+            if (in_array($filterName, ['ct.id_customer_thread', 'ct.private'])) {
+                $builder->andWhere('z.' . $filterName . ' = :' . $filterName);
+                $builder->setParameter($filterName, $filterValue);
+                continue;
+            }
+
+            if (in_array($filterName, ['message', 'private'])) {
+                $builder->andWhere('cm.' . $filterName . ' LIKE :' . $filterName);
+                $builder->setParameter($filterName, '%' . $filterValue . '%');
+                continue;
+            }


Seems like private is handled twice and differently no?

Besides I believe there is no table that is aliased to z, and ['ct.id_customer_thread', 'ct.private'] are not in the allowed filters so the block should never be reached right?

src/PrestaShopBundle/Resources/config/routing/admin/sell/customer_service/customer_threads.yml

jolelievre · 2022-10-19T09:59:47Z

src/PrestaShopBundle/Controller/Admin/Sell/CustomerService/CustomerThreadController.php

     *
     * @return RedirectResponse
     */
-    public function updateStatusAction($customerThreadId, $newStatus)
+    public function updateStatusAction(int $customerThreadId, Request $request)


Suggested change

public function updateStatusAction(int $customerThreadId, Request $request)

public function updateStatusFromViewAction(int $customerThreadId, Request $request)

I suggest renaming this controller to understand that two controllers are needed because they don't redirect to the same place based on the context they have been used

jolelievre · 2022-10-19T10:00:17Z

src/PrestaShopBundle/Resources/config/routing/admin/sell/customer_service/customer_threads.yml

@@ -19,7 +38,7 @@ admin_customer_threads_reply:
    customerThreadId: \d+

 admin_customer_threads_update_status:


Suggested change

admin_customer_threads_update_status:

admin_customer_threads_view_update_status:

jolelievre · 2022-10-19T10:02:56Z

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

@@ -34,3 +34,24 @@ services:
    tags:
      - name: tactician.handler
        command: 'PrestaShop\PrestaShop\Core\Domain\CustomerService\Command\ReplyToCustomerThreadCommand'
+
+  prestashop.adapter.customer_service.command_handler.delete_customer_thread_handler:


As suggested in a previous comment all those new handlers could be added to the core namespace instead
Maybe some others that you created recently could also be moved, but it should be handled in another PR

zuk3975 · 2022-10-27T08:09:38Z

src/Core/Domain/CustomerService/Command/BulkDeleteCustomerThreadCommand.php

+     *
+     * @throws CustomerServiceException
+     */
+    public function setCustomerThreadIds(array $customerThreadIds)


return void, or I guess even bettter - self to make it fluent setter

src/Core/Domain/CustomerService/Command/BulkDeleteCustomerThreadCommand.php

zuk3975 · 2022-10-27T08:12:10Z

src/Core/Domain/CustomerService/CommandHandler/BulkDeleteCustomerThreadHandler.php

+        $this->customerThreadRepository = $customerThreadRepository;
+    }
+
+    public function handle(BulkDeleteCustomerThreadCommand $command)


Suggested change

public function handle(BulkDeleteCustomerThreadCommand $command)

public function handle(BulkDeleteCustomerThreadCommand $command): void

zuk3975 · 2022-10-27T08:12:23Z

src/Core/Domain/CustomerService/CommandHandler/BulkDeleteCustomerThreadHandlerInterface.php

+ */
+interface BulkDeleteCustomerThreadHandlerInterface
+{
+    public function handle(BulkDeleteCustomerThreadCommand $command);


Suggested change

public function handle(BulkDeleteCustomerThreadCommand $command);

public function handle(BulkDeleteCustomerThreadCommand $command): void;

zuk3975 · 2022-10-27T08:12:48Z

src/Core/Domain/CustomerService/CommandHandler/DeleteCustomerThreadHandlerInterface.php

+ */
+interface DeleteCustomerThreadHandlerInterface
+{
+    public function handle(DeleteCustomerThreadCommand $command);


Suggested change

public function handle(DeleteCustomerThreadCommand $command);

public function handle(DeleteCustomerThreadCommand $command): void;

zuk3975 · 2022-10-27T08:15:32Z

src/Core/Domain/CustomerService/DataProvider/ContactTypeProvider.php

+
+use Contact;
+
+class ContactTypeProvider


isn't it a simple form choice provider as others?
Even if the select form is in grid, I think its still ok to have this under the same directory as other form choice providers

zuk3975 · 2022-10-27T08:22:45Z

src/Core/Grid/Data/Factory/CustomerThreadGridDataFactoryDecorator.php

+    /**
+     * {@inheritdoc}
+     */
+    public function getData(SearchCriteriaInterface $searchCriteria)


I guess there is no return type, because of the old interface, but Im pretty sure you could add the return type in this class and it would work 🤔 (It breaks only when interface defines different return type, but it works if its not defined at all). Same idea for the PrivateColumn.php above.

zuk3975

just the concern about the form choice provider and some missing return types.

jolelievre

Thanks @margud

Just a few minor things to fix and it's all good

However, you didn't answer my question about the complex subquery, can you explain this a bit more and maybe add a comment if it is really required? Because I feel like the subquery is overly complex, and also it seems like the same thread can be returned multiple times (see my comments for more details).

jolelievre · 2022-10-27T11:07:31Z

src/Adapter/Form/ChoiceProvider/CustomerThreadStatusesChoiceProvider.php

+use PrestaShop\PrestaShop\Core\Form\ConfigurableFormChoiceProviderInterface;
+use Symfony\Contracts\Translation\TranslatorInterface;
+
+class CustomerThreadStatusesChoiceProvider implements ConfigurableFormChoiceProviderInterface


If you don't need to pass any options or parameters then the FormChoiceProviderInterface is enough

Suggested change

class CustomerThreadStatusesChoiceProvider implements ConfigurableFormChoiceProviderInterface

class CustomerThreadStatusesChoiceProvider implements FormChoiceProviderInterface

well when i change it to formChoice i get this error:

Yeah because you probably didn't update the grid factory that relies on it

jolelievre · 2022-10-27T11:07:46Z

src/Adapter/Form/ChoiceProvider/CustomerThreadStatusesChoiceProvider.php

+        $this->translator = $translator;
+    }
+
+    public function getChoices(array $options = [])


Suggested change

public function getChoices(array $options = [])

public function getChoices()

can't do that because of interface

That's of course once you change the interface

src/Core/Domain/CustomerService/Command/BulkDeleteCustomerThreadCommand.php

jolelievre · 2022-10-27T11:27:18Z

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

@@ -2,8 +2,8 @@ services:
  _defaults:
    public: true

-  prestashop.adapter.customer_message.command_handler.add_order_customer_message:
-    class: PrestaShop\PrestaShop\Adapter\CustomerService\CommandHandler\AddOrderCustomerMessageHandler
+  prestashop.core.domain.customer_service.command_handler.add_order_customer_message:


Actually, this one (along with all the other moved handlers) should now be placed into https://github.com/PrestaShop/PrestaShop/blob/develop/src/PrestaShopBundle/Resources/config/services/core/domain/customer_service.yml

jolelievre · 2022-10-27T11:29:00Z

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

@@ -0,0 +1,19 @@
+services:


This file should be moved into src/PrestaShopBundle/Resources/config/services/core/domain/customer_service.yml instead
For the handler definitions, we put them in the domain subfolder (the file already exists by the way)

Again the handlers service definition should be in the domain subfolder to separate them from the "regular" services (like repositories, updater, modifier, ...) which are potentially used by the handlers themselves.

But since the definition of CQRS handlers is quite verbose it's better to keep them separated.

This file still needs to be moved in domain subfolder

src/Core/Grid/Query/CustomerThreadQueryBuilder.php

jolelievre · 2022-10-27T13:11:22Z

src/Core/Grid/Query/CustomerThreadQueryBuilder.php

+    {
+        $qb = $this->getQueryBuilder($searchCriteria)
+            ->select('ct.*, CONCAT(c.`firstname`," ",c.`lastname`) as customer')
+            ->addSelect('cm.message, cm.private, cl.name as contact, l.name as langName')


I noticed a different between the migrated grid and the legacy one regarding message column:

Before it displayed all messages (but truncated) now it displays only the first message. I'm not sure either of these behaviour is correct IMHO, I think it should only display the last message (truncated as well of course)

The last message truncated is enough IMO as well

Ok, thanks @MatShir then you'll have to update the specs or the QA won't accept this change.

So @margud you need to add an orderBy condition to get the latest message in jour join. About the truncate maybe it should be handled in the grid rendering but I don't know if we have a column type that handles this, it could be interesting I think. Or maybe even better we can add an option to DataColumn max_displayed_characters or something, and handle it in the twig This way e can reuse it in any other grid.

jolelievre · 2022-10-27T13:19:13Z

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

@@ -93,3 +93,8 @@ services:
      - '@prestashop.adapter.attribute.repository.attribute_repository'
      - '@prestashop.core.product.combination.name_builder.combination_name_builder'
      - '@=service("prestashop.adapter.legacy.context").getLanguage().id'
+
+  prestashop.adapter.form.choice_provider.contact_type_choice_provider:


This definition should be in the choice_provider.yml file, it's not a form type it's a choice provider

this one is form choice provider

i guess i can move them from adapter to core aswell, then it should be in choice_provider.yml

Nearly all choice provider are used by form types actually, but we still separate them because we have a lot of choice providers and a lot of form types So we try to keep things (a bit) clean 😅

jolelievre

Thanks @margud

Since @MatShir answered there is just a little bug about the message column to handle. And one service file to move in a subfolder and we're good!

jolelievre · 2022-10-27T15:36:03Z

src/Core/Grid/Query/CustomerThreadQueryBuilder.php

+    {
+        $qb = $this->getQueryBuilder($searchCriteria)
+            ->select('ct.*, CONCAT(c.`firstname`," ",c.`lastname`) as customer')
+            ->addSelect('cm.message, cm.private, cl.name as contact, l.name as langName')


Ok, thanks @MatShir then you'll have to update the specs or the QA won't accept this change.

So @margud you need to add an orderBy condition to get the latest message in jour join. About the truncate maybe it should be handled in the grid rendering but I don't know if we have a column type that handles this, it could be interesting I think. Or maybe even better we can add an option to DataColumn max_displayed_characters or something, and handle it in the twig This way e can reuse it in any other grid.

jolelievre · 2022-10-27T15:37:57Z

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

@@ -0,0 +1,19 @@
+services:


Again the handlers service definition should be in the domain subfolder to separate them from the "regular" services (like repositories, updater, modifier, ...) which are potentially used by the handlers themselves.

But since the definition of CQRS handlers is quite verbose it's better to keep them separated.

jolelievre

Thanks @margud

Only two small things to fix and I think we're good for a first merge of this initial PR

jolelievre · 2022-10-28T13:25:14Z

src/PrestaShopBundle/Resources/views/Admin/Common/Grid/Columns/Content/data.html.twig


-{{ record[column.options.field] }}
+{% if column.options.max_displayed_characters > 0 %}
+  {{ record[column.options.field]|slice(0, column.options.max_displayed_characters) }}


Suggested change

{{ record[column.options.field]|slice(0, column.options.max_displayed_characters) }}

{{ record[column.options.field]|slice(0, column.options.max_displayed_characters) }}...

or maybe better

Suggested change

{{ record[column.options.field]|slice(0, column.options.max_displayed_characters) }}

{{ record[column.options.field]|slice(0, column.options.max_displayed_characters) }}&mldr;

But we need to show that the content was truncated

jolelievre · 2022-10-28T13:26:06Z

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

@@ -0,0 +1,19 @@
+services:


This file still needs to be moved in domain subfolder

jolelievre

Just one little bug to fix

jolelievre · 2022-10-28T13:55:04Z

src/PrestaShopBundle/Resources/views/Admin/Common/Grid/Columns/Content/data.html.twig


-{{ record[column.options.field] }}
+{% if column.options.max_displayed_characters > 0 %}


I just tested it and the ellipsis is always shown I think a condition is missing here

Suggested change

{% if column.options.max_displayed_characters > 0 %}

{% if column.options.max_displayed_characters > 0 and column.options.field|length > column.options.max_displayed_characters %}

Not sure about |length filter, but something similar ^^

jolelievre · 2022-10-28T13:57:11Z

src/PrestaShopBundle/Resources/views/Admin/Common/Grid/Columns/Content/data.html.twig

@@ -1,4 +1,4 @@
-{#**
+{# **


Suggested change

{# **

{#**

jolelievre · 2022-10-28T13:57:22Z

src/PrestaShopBundle/Resources/views/Admin/Common/Grid/Columns/Content/data.html.twig

@@ -21,6 +21,10 @@
 * @author    PrestaShop SA and Contributors <[email protected]>
 * @copyright Since 2007 PrestaShop SA and Contributors
 * @license   https://opensource.org/licenses/OSL-3.0 Open Software License (OSL 3.0)
- *#}
+ * #}


Suggested change

* #}

*#}

Also, don't add spaces here, or I believe our prestashop:licenses:update command will not detect the license correctly and it will add another one

jolelievre

Thanks @margud

jolelievre · 2022-11-03T12:12:47Z

Thanks @margud

I merged this PR without QA because it's only part of the page, a few other PRs are needed to finish it and the last one will be tested by QA team

finished grid and improved error handle for all actions

3148a2a

margud requested a review from a team as a code owner September 23, 2022 12:50

prestonBot added the Refactoring Type: Refactoring label Sep 23, 2022

margud added 8 commits September 23, 2022 16:27

improved code, fixed employer filter

71ef63e

reverted cs changes

663ff63

finished grid with deletion and view logic

5ad361a

finished grid

e2c78fb

issue fixes

ef278a3

cs fix

5a7470b

style fix

96e90bb

phpstan fixes

fd90b71

margud commented Oct 19, 2022

View reviewed changes

src/PrestaShopBundle/Resources/config/routing/admin/sell/customer_service/customer_threads.yml Show resolved Hide resolved

improved code

a11dabc

prestonBot added the develop Branch label Oct 19, 2022

cs fixer

ea52e24

jolelievre reviewed Oct 19, 2022

View reviewed changes

margud added 6 commits October 20, 2022 13:06

code review improvments

94c46c6

review changes

57559e2

php stan fix

53b9ded

code review refactor

b512651

cs fix

48e48e8

cs fixer

2dc8faa

zuk3975 reviewed Oct 27, 2022

View reviewed changes

src/Core/Domain/CustomerService/Command/BulkDeleteCustomerThreadCommand.php Show resolved Hide resolved

zuk3975 reviewed Oct 27, 2022

View reviewed changes

zuk3975 requested changes Oct 27, 2022

View reviewed changes

margud added 2 commits October 27, 2022 13:12

code review fix

aab5a58

review fix

327d3e0

zuk3975 previously approved these changes Oct 27, 2022

View reviewed changes

jolelievre reviewed Oct 27, 2022

View reviewed changes

code review changes

63548d6

margud dismissed zuk3975’s stale review via 63548d6 October 27, 2022 13:01

jolelievre reviewed Oct 27, 2022

View reviewed changes

moved new providers to core

456304b

jolelievre reviewed Oct 27, 2022

View reviewed changes

code review improvments

ad57206

jolelievre reviewed Oct 28, 2022

View reviewed changes

review fixes

92389d0

jolelievre reviewed Oct 28, 2022

View reviewed changes

data twig file fix

031fa32

jolelievre approved these changes Nov 3, 2022

View reviewed changes

zuk3975 approved these changes Nov 3, 2022

View reviewed changes

jolelievre merged commit 91395e8 into PrestaShop:develop Nov 3, 2022

kpodemski added this to the 8.1.0 milestone Feb 13, 2023

	public function updateStatusAction(int $customerThreadId, Request $request)
	public function updateStatusFromViewAction(int $customerThreadId, Request $request)

		@@ -19,7 +38,7 @@ admin_customer_threads_reply:
		customerThreadId: \d+

		admin_customer_threads_update_status:

	admin_customer_threads_update_status:
	admin_customer_threads_view_update_status:

	public function handle(BulkDeleteCustomerThreadCommand $command)
	public function handle(BulkDeleteCustomerThreadCommand $command): void

	public function handle(DeleteCustomerThreadCommand $command);
	public function handle(DeleteCustomerThreadCommand $command): void;

	class CustomerThreadStatusesChoiceProvider implements ConfigurableFormChoiceProviderInterface
	class CustomerThreadStatusesChoiceProvider implements FormChoiceProviderInterface

	public function getChoices(array $options = [])
	public function getChoices()

	{{ record[column.options.field]\|slice(0, column.options.max_displayed_characters) }}
	{{ record[column.options.field]\|slice(0, column.options.max_displayed_characters) }}...


		{{ record[column.options.field] }}
		{% if column.options.max_displayed_characters > 0 %}

Customer service controller migration (grid part) #29732

Customer service controller migration (grid part) #29732

Conversation

margud commented Sep 23, 2022 • edited Loading

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

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

zuk3975 left a comment • edited Loading

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

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

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

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

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

jolelievre left a comment

Choose a reason for hiding this comment

jolelievre commented Nov 3, 2022

margud commented Sep 23, 2022 •

edited

Loading

zuk3975 left a comment •

edited

Loading