New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor ps_linklist module using 1.7 improvements #48

Merged
merged 44 commits into from Nov 9, 2018

Conversation

@jolelievre
Contributor

jolelievre commented Aug 30, 2018

Questions Answers
Branch? develop
Description? Symfony migration of the module using the new Grid component, namespaces, Symfony controllers, ...
Type? improvement
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #9746
How to test? You need to clone the module and select the migrate-sf branch, BE CAREFULL you also need to use the link-modules branch https://github.com/jolelievre/PrestaShop as there are required changes in the Core (cf PrestaShop/PrestaShop#10414) Then try to uninstall and reinstall the module and finally test the module in Design > Link Widget

This change is Reviewable

return $cmsBlock;
}
public function getCmsPages($langId = null, $shopId = null)

This comment has been minimized.

@kpodemski
@@ -0,0 +1,35 @@
<?php
/*
* 2007-2016 PrestaShop

This comment has been minimized.

@matks

matks Sep 2, 2018

Old license header ;)

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Sep 2, 2018

ping @sarjon, would you mind to review this one this week?

@joelelievre good job 👍

@sarjon

minor comments, keep up the good work. 👍

},
"autoload": {
"psr-4": {
"PrestaShop\\LinkList\\": "src/"

This comment has been minimized.

@sarjon

sarjon Sep 2, 2018

Member

wouldnt it be better to call namepsace PrestaShop\\Module\\LinkList\\ for modules?

This comment has been minimized.

@jolelievre

jolelievre Sep 3, 2018

Contributor

that was my initial namespace, I changed it because I wasn't sure of the usual used one
I'm gonna go back to PrestaShop\\Module\\LinkList\\

defaults:
_controller: 'PrestaShop\LinkList\Controller\Admin\Improve\Design\LinkWidgetController::listAction'
_legacy_controller: AdminLinkWidget
# _legacy_link: AdminLinkWidget

This comment has been minimized.

@sarjon

sarjon Sep 2, 2018

Member

dont forget to delete this unless you use it. :)

This comment has been minimized.

@jolelievre

jolelievre Sep 3, 2018

Contributor

it is one of the two solutions I experimented to update the Link::createUrl method, we discussed about it with @mickaelandrieu and it will be used I will link the PR from the core when I have cleaned it

_controller: 'PrestaShop\LinkList\Controller\Admin\Improve\Design\LinkWidgetController::editAction'
_legacy_controller: AdminLinkWidget
admin_link_block_create:

This comment has been minimized.

@sarjon

sarjon Sep 2, 2018

Member

this is confusing, you have admin_link_block_new route with newAction() for form display, right? and here is admin_link_block_create route with action createAction for form processing? if so, i would prefer having similar naming, for example:

admin_link_block_create route and createAction() for display and admin_link_block_create_process with createProcessAction() for processing, wdyt? same for editing. :)

This comment has been minimized.

@jolelievre

jolelievre Sep 3, 2018

Contributor

ok, I am fin with your naming

@@ -0,0 +1,101 @@
parameters:
hello_world: "Hello world!"

This comment has been minimized.

@sarjon

sarjon Sep 2, 2018

Member

do you use it?

This comment has been minimized.

@jolelievre

jolelievre Sep 3, 2018

Contributor

to be removed, the PR is still a work in progress, so it's full of useless things I didn't take time to clean yet

hello_world: "Hello world!"
services:
prestashop.link_block.repository:

This comment has been minimized.

@sarjon

sarjon Sep 2, 2018

Member

if you add Module to namespace, please dont forget to update keys to prestashop.module.link_block.*.

public function __construct(FormChoiceProviderInterface $cmsCategoriesProvider)
{
$this->cmsCategoriesProvider = $cmsCategoriesProvider;

This comment has been minimized.

@sarjon

sarjon Sep 2, 2018

Member

you can inject array instead of service.

FormChoiceProviderInterface $hookChoiceProvider,
FormChoiceProviderInterface $cmsPageChoiceProvider,
FormChoiceProviderInterface $productPageChoiceProvider,
FormChoiceProviderInterface $staticPageChoiceProvider

This comment has been minimized.

@sarjon

sarjon Sep 2, 2018

Member

same as before, inject choices as arrays instead of services. 👍

@@ -64,8 +67,8 @@ public function dropTables()
public function getCMSBlocksSortedByHook($id_shop = null, $id_lang = null)
{
$id_lang = (int) (($id_lang) ?: Context::getContext()->language->id);
$id_shop = (int) (($id_shop) ?: Context::getContext()->shop->id);
$id_lang = (int) (($id_lang) ?: \Context::getContext()->language->id);

This comment has been minimized.

@sarjon

sarjon Sep 2, 2018

Member

i think you dont need parentheses around $id_lang. Please use camelCase for naming. :)

This comment has been minimized.

@jolelievre

jolelievre Sep 3, 2018

Contributor

this is the legacy repository, I didn't change it (except for the namespace modification) but it is supposed to be removed once the new controller is finished

$id_lang = (int) (($id_lang) ?: Context::getContext()->language->id);
$id_shop = (int) (($id_shop) ?: Context::getContext()->shop->id);
$id_lang = (int) (($id_lang) ?: \Context::getContext()->language->id);
$id_shop = (int) (($id_shop) ?: \Context::getContext()->shop->id);

This comment has been minimized.

@sarjon

sarjon Sep 2, 2018

Member

same here as above.

use Symfony\Component\Translation\TranslatorInterface as Translator;
use PrestaShop\LinkList\Model\LinkBlock;
class LinkBlockRepository

This comment has been minimized.

@sarjon

sarjon Sep 2, 2018

Member

this class has way too many responsibilities, consider refactoring.

This comment has been minimized.

@jolelievre

jolelievre Sep 3, 2018

Contributor

yes it is an ugly copy/paste of the legacy class that still needs to be cleaned ^^

@mickaelandrieu mickaelandrieu changed the title from WIP: Migrate sf to WIP: refactored ps_linklist module to use PS 1.7 improvements Sep 3, 2018

@mickaelandrieu mickaelandrieu changed the title from WIP: refactored ps_linklist module to use PS 1.7 improvements to WIP: refactored ps_linklist module using 1.7 improvements Sep 3, 2018

@@ -40,3 +40,10 @@ admin_link_block_delete:
defaults:
_controller: 'PrestaShop\Module\LinkList\Controller\Admin\Improve\Design\LinkWidgetController::deleteAction'
_legacy_controller: AdminLinkWidget
admin_link_block_update_positions:
path: /link_widget/update_positions

This comment has been minimized.

@PierreRambaud

PierreRambaud Sep 12, 2018

Contributor

any reason to use underscore instead of hyphen?

This comment has been minimized.

@jolelievre

jolelievre Sep 12, 2018

Contributor

absolutely none ^^

This comment has been minimized.

@PierreRambaud

PierreRambaud Sep 12, 2018

Contributor

It's always better to use hyphen instead of underscore in urls ;) (even if this page isn't in seo context, let's keep good practices :D)

private function getLinkBlockRepository()
{
if (null === $this->repository) {
$this->repository = new LegacyLinkBlockRepository(

This comment has been minimized.

@PierreRambaud

PierreRambaud Sep 12, 2018

Contributor

Possibility to use an Adapter with symfony service?

This comment has been minimized.

@jolelievre

jolelievre Sep 12, 2018

Contributor

this controller will be removed once the whole migration is done

@@ -103,6 +109,14 @@ public function uninstallTab()
return $tab->delete();
}
public function hookActionSymfonyModuleRoutes()
{
return [];

This comment has been minimized.

@PierreRambaud

PierreRambaud Sep 12, 2018

Contributor

Double return watch out

This comment has been minimized.

@jolelievre

jolelievre Sep 12, 2018

Contributor

this was a test for module route management, this implementation was know kept But thanks I forgot to remove it

*/
public function __construct()
{
$this->module = \Module::getInstanceByName('ps_linklist');

This comment has been minimized.

@PierreRambaud

PierreRambaud Sep 12, 2018

Contributor

PrestaShop\PrestaShop\Core\Addon\Module\ModuleRepository->getInstanceByName is maybe better?

This comment has been minimized.

@Quetzacoalt91

Quetzacoalt91 Sep 12, 2018

Member

or PrestaShop\PrestaShop\Core\Addon\Module\ModuleRepository->getModule('ps_linklist')->getInstance();

This comment has been minimized.

@jolelievre

jolelievre Sep 12, 2018

Contributor

yes again

$grids = $linkBlockGridFactory->getGrids($hooks, $filtersParams);
/** @var GridPresenter $gridPresenter */
$gridPresenter = $this->get('prestashop.core.grid.presenter.grid_presenter');

This comment has been minimized.

@PierreRambaud

PierreRambaud Sep 12, 2018

Contributor

The presenter could contains a presentCollection method?

This comment has been minimized.

@jolelievre

jolelievre Sep 12, 2018

Contributor

I am not sure if it is useful This behaviour is really specific to this module because we have multiple grid per hooks In most cases there is only one grid, even my LinkBlockGridFactory is kinda out of the regular scope
But if we think that we will regularly have multiple grids in a single page then this might be useful indeed, @mickaelandrieu wdyt?

$form = $formHandler->getForm();
$form->handleRequest($request);
if ($form->isSubmitted()) {

This comment has been minimized.

@PierreRambaud

PierreRambaud Sep 12, 2018

Contributor

Missing $form->isValid(); ?

This comment has been minimized.

@jolelievre

jolelievre Sep 12, 2018

Contributor

no validator here but it doesn't cost much

This comment has been minimized.

@PierreRambaud

PierreRambaud Sep 12, 2018

Contributor

It's just in case someone create a module to change this module and add validator :trollface:
But otherwise, we still make some data required when saving form? What happened if you send no data?

This comment has been minimized.

@jolelievre

jolelievre Sep 12, 2018

Contributor

yes that's why I added the isValid, just in case
Even though in this case the validation is performed by the FormHandler in saveData method, not a big fan of this though maybe the Interface should implement a validateData method

}
return [
'linkBlockForm' => $form->createView(),

This comment has been minimized.

@PierreRambaud

PierreRambaud Sep 12, 2018

Contributor

You keep the $form->createView() what is the interest to have another view to GET the form?

This comment has been minimized.

@jolelievre

jolelievre Sep 12, 2018

Contributor

I'm not sure I get what you mean I keep the form displaying in createProcess/editProcess to not lose the previously submitted data
And I could factorize the four endpoints create[GET,POST] and edit[GET,POST] but I find it cleaner to keep each end points separeted, yet I factorized the private function processForm since in this case they are managed the same way This might change when the new EntityForm will be available

Which makes me think that I have no hook for creation/update But there were none in the previous module Maybe this can wait the refacto where the hooks will be managed "natively" in the core

->leftJoin('lb', $this->dbPrefix . 'hook', 'h', 'lb.id_hook = h.id_hook');
foreach ($filters as $name => $value) {
if ('id_lang' === $name) {

This comment has been minimized.

@PierreRambaud

PierreRambaud Sep 12, 2018

Contributor

you can remove foreach and edit your if conditions

if (isset($filters['id_lang'])):
if (isset($filters['id_hook'])):

or

foreach (['id_lang', 'id_hook'] as $name) {
  if (!isset($filters[$name])) {
    continue;
  }

  $qb
    ->andWhere(sprintf('h.%s = :%s', $name, $name))
    ->setParameter($name, $filters[$name]);
}

This comment has been minimized.

@jolelievre

jolelievre Sep 12, 2018

Contributor

I inspired myself from the existing query builders from the Core, I think the aim of the loop here is that if someone extends the definition and adds a field or a filter there is no need to update the Query builder hence the loop
@mickaelandrieu do you confirm?

This comment has been minimized.

@PierreRambaud

PierreRambaud Sep 19, 2018

Contributor

But with what you did you're only authorized to have id_lang and id_hook too =)

This comment has been minimized.

@jolelievre

jolelievre Sep 19, 2018

Contributor

that's quite pertinent! :D

@jolelievre

This comment has been minimized.

Contributor

jolelievre commented Oct 18, 2018

ok the modif is done, I am waiting for your reviews please
PrestaShop/PrestaShop#11063
@tomas862 @matks @PierreRambaud

I actually think it is not more complex at all, it even seems simpler and clearer

/**
* @param Request $request
* @param string $hookId

This comment has been minimized.

@tomas862

tomas862 Oct 18, 2018

Member

I guess it's int type? 🤔

Suggested change Beta
* @param string $hookId
* @param int $hookId

This comment has been minimized.

@jolelievre

jolelievre Oct 18, 2018

Contributor

yep, you're right!

@marionf

This comment has been minimized.

marionf commented Oct 19, 2018

@jolelievre

Seen with @TristanLDD there are some UX "issues"

  • drag & drop @TristanLDD will send you the mock-ups

  • checkboxes should be like this

capture d ecran_486

Instead of this

capture d ecran_485

  • drop down should be like this

capture d ecran_484

Instead of this

capture d ecran_487

You can see UI-kit for the drop down and checkboxes: https://cdn.rawgit.com/PrestaShop/prestashop-ui-kit/master/index.html

  • The form should be opened when I arrive on the page

capture d ecran_488

Otherwise, everything is good :)

private function isEmptyCustom(array $custom)
{
foreach ($custom as $langCustom) {
$fields = ['title', 'url'];

This comment has been minimized.

@PierreRambaud

PierreRambaud Nov 6, 2018

Contributor

Don't declare this each time you loop :)

@matks

This comment has been minimized.

matks commented Nov 9, 2018

@jolelievre PrestaShop/PrestaShop#11147 has been merged, you should be able to finish this PR 😃

@jolelievre

This comment has been minimized.

Contributor

jolelievre commented Nov 9, 2018

yep, fingers crossed!!! 😬🤞

@marionf marionf added QA ✔️ and removed Waiting for author labels Nov 9, 2018

@PierreRambaud PierreRambaud merged commit 6ff0c2b into PrestaShop:dev Nov 9, 2018

@PierreRambaud

This comment has been minimized.

Contributor

PierreRambaud commented Nov 9, 2018

Thanks @jolelievre

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