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

Migrate categories listing #10160

Merged
merged 87 commits into from Oct 8, 2018

Conversation

@sarjon
Member

sarjon commented Aug 27, 2018

Questions Answers
Branch? develop
Description? Migrates categories listing page
Type? new feature
Category? CO
BC breaks? no
Deprecations? no
Fixed ticket? #10193
How to test? See below.

Access "Catalog > Categories" page. All actions related to list are migrated except for:

  • Add new still redirects to legacy page (will be migrated in next PR)
  • Edit also redirects to legacy page (will be migrated in next PR)
  • Export action is not fully implemented as it does not consider filters (will be fixed in other PR, here's an issue #10293)

This change is Reviewable

@sarjon

This comment has been minimized.

Member

sarjon commented Aug 27, 2018

@mickaelandrieu wdyt about this commit? 810de92

also this one d481ee4 it's a workaround for modifiers that we talked earlier, wdyt? Maybe there's better approach?

/**
* Class SearchCriteriaWithCategoryParentIdFilterFactory
*/
final class SearchCriteriaWithCategoryParentIdFilterFactory implements DecoratedSearchCriteriaFactory

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Aug 28, 2018

Contributor

we shouldn't have to "hack" the request of the user, but if we do it we should do it before entering the controller.

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Aug 28, 2018

Contributor

we could dispatch an event and a Hook in Controller Argument Resolver, wdyt?

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Aug 28, 2018

Contributor

about resolving the data of SearchCriteria/Filters instance

This is why I suggest to add an Hook and an Event here https://github.com/PrestaShop/PrestaShop/blob/develop/src/PrestaShopBundle/Controller/ArgumentResolver/SearchParametersResolver.php#L103

This comment has been minimized.

@sarjon

sarjon Aug 28, 2018

Member

sure, event sounds good. 👍 but arent Filters immutable? we should be able to extend them.

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Aug 28, 2018

Contributor

we should be able to extend them.

why? you are not unable to replace the existing one in the controller and Symfony controllers are not overridable ^^

Also creating your own using the interface or the abstract class is really easy, isn't it?

This comment has been minimized.

@sarjon

sarjon Aug 28, 2018

Member

so, what part should we change? 🤔

 // we get $filters (SearhcCriteriaInterface) that are resolved in SearchParametersResolver
public function indexAction(CategoryFilters $filters) 
{
    // here we decorate resolved $filters with dynamic `id_category_parent` that is not saved with filters as it is not part of filtering form
    $searchCriteriaFactory = $this->get('prestashop.adapter.grid.search.factory.search_criteria_with_category_parent_id');
    $searchCriteria = $searchCriteriaFactory->createFrom($filters);

    // usual workflow below
    // ...
}

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Aug 28, 2018

Contributor

For me, the injected $filters in action should already have the resolved value of id_category_parent.

So, you can dispatch an event (and a hook) in SearchParametersResolver, and use an event listener update the object before it was sent inside the action arguments.

So, it's not about changing this code, but moving it outside of the controller :)

This comment has been minimized.

@sarjon

sarjon Aug 28, 2018

Member

hmm, it makes sense. 👍

use Doctrine\DBAL\Query\QueryBuilder;
use PrestaShop\PrestaShop\Core\Grid\Search\SearchCriteriaInterface;
final class CategoryQueryBuilder extends AbstractDoctrineQueryBuilder

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Aug 28, 2018

Contributor

PHPDoc

/**
* Interface DecoratedSearchCriteriaFactory defines contract for decorated search criteria factory
*/
interface DecoratedSearchCriteriaFactory

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Aug 28, 2018

Contributor

I don't think we should try to override that comes from the user, but maybe we need it? I need to be convinced

This comment has been minimized.

@sarjon

sarjon Aug 28, 2018

Member

it does not override what comes from user, what it does is keeps exactly the same data except it extends criteria with dynamic parent_category_id value. :)

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Aug 28, 2018

Contributor

yes but resolving the data inside SearchCriteria is not "their" responsibility, this responsibility is inside a Controller Argument Resolver: the way I see it, SearchCriteria is immutable and the responsibility to build it is already in SearchParameterResolver.

use PrestaShop\PrestaShop\Core\Search\Filters;
class CategoryFilters extends Filters

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Aug 28, 2018

Contributor

PHPDoc

if (is_callable($options['is_applicable_callback'])) {
return call_user_func($options['is_applicable_callback'], [$record]);
if ($accessibilityChecker instanceof AccessibilityCheckerInterface) {

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Sep 4, 2018

Contributor

looks like we could have a hook here for instance, and the only expected return is a boolean so people can define their own rules in module.

just an idea: I don't think we should do it right now (we need feedbacks from developers after 1.7.5 release)

/**
* {@inheritdoc}
*/
public static function getDefaults()

This comment has been minimized.

@sarjon

sarjon Sep 5, 2018

Member

does it have to be static? it's weird having interface with static method, isnt it? 🤔

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Sep 5, 2018

Contributor

The interface has been created after the implementation, there is nothing from with static methods if used right :)

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Sep 5, 2018

Contributor

why? An interface defines a contract and we needs this public static method.

$currentCategoryId = $filters->getFilters()['id_category_parent'];
$categoryViewDataProvider = $this->get('prestashop.adapter.category.category_view_data_provider');
$categoryViewData = $categoryViewDataProvider->getViewData($currentCategoryId);

This comment has been minimized.

@sarjon

sarjon Sep 5, 2018

Member

i think GetCategoryForListViewQuery or similar should be used instead of CategoryViewDataProvider adapter? @eternoendless what do you think?

Use case:
Once you open Categories list, it resolves parent category for which list of child categories are displayed. By default, it uses Home category and displays all of it children. So currently CategoryViewDataProvider:getViewData($parentCategoryId) returns this kind of data:

return [
    'breadcrumb_tree' => $categoriesTree, // used to display breadrumb
    'id' => $category->id,
    'id_parent' => $category->id_parent,
    'is_home_category' => $this->configuration->get('PS_HOME_CATEGORY') == $category->id,
    'name' => $category->name[$this->contextLangId], // used to display layout title
];

Im thinking that Query could return some DTO with this data instead of array.

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Sep 5, 2018

Contributor

note for any reviewer: get the pull request and try on your own computer, this grid is a little bit different from what we use 😕

@sarjon

This comment has been minimized.

Member

sarjon commented Sep 5, 2018

@mickaelandrieu @PierreRambaud waiting for review. 👍 @eternoendless can you take a look at 2 Commands that are implemented in this PR?

@mickaelandrieu mickaelandrieu changed the title from [WIP] Migrate categories listing to Migrate categories listing Sep 5, 2018

@prestonBot prestonBot added the develop label Sep 5, 2018

@matks

matks approved these changes Oct 8, 2018

@matks matks removed the waiting for QA label Oct 8, 2018

@matks

This comment has been minimized.

Contributor

matks commented Oct 8, 2018

Merged as it is too big
PR unprocessed feedbacks and QA validation will happen in another PR

@matks matks merged commit e6a1a4c into PrestaShop:develop Oct 8, 2018

2 of 3 checks passed

code-review/reviewable 131 files, 54 discussions left (eternoendless, LouiseBonnard, matks, mickaelandrieu, PierreRambaud, Quetzacoalt91, sarjon)
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment