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

Introduced Grid system #8990

Merged
merged 49 commits into from Jun 28, 2018

Conversation

@sarjon
Member

sarjon commented Apr 25, 2018

Questions Answers
Branch? develop
Description? Grid is a new component to abstract and ease the migration of all CRUD-based page. This contribution is provided with the component and the implementation of Logs page.
Type? new feature
Category? CO
BC breaks? no
Deprecations? no
Fixed ticket? n/a
How to test? Access admin-dev/index.php/configure/advanced/logs and - after the security access page - you should see the logs page. Be careful, for now, the search doesn't works and its part of #8984. Note bis: the Logs page won't be directly accessible until the merge of #8984 .

===> Docs <===


This change is Reviewable

@sarjon sarjon changed the title from [WIP] Introduced Dynamic grid system to [WIP] Introduced Grid system Apr 25, 2018

@prestonBot prestonBot added the develop label Apr 25, 2018

@mickaelandrieu mickaelandrieu added this to the 1.7.5.0 milestone Apr 25, 2018

{
// if form type is set for column
// then column is filterable
return $this->getFilterFormType() ? true : false;

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Apr 25, 2018

Contributor
- return $this->getFilterFormType() ? true : false;
+ return null !== $this->getFilterFormType();
*
* @param array $filters
*
* @return array

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Apr 25, 2018

Contributor

I'd like to see a real RowsCollection here, using an object is easier for validation.

If one property is missing on your list, or if we want to allow a specific behavior for one column, it's better to manage it in his own object. (It wasn't finished, but you can spy ItemCollection/ItemInterface/Item from my PR)

This comment has been minimized.

@sarjon

sarjon Apr 27, 2018

Member

getRows() returns rows data from database (in most cases), are you sure collection in necessary here?

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 3, 2018

Contributor

Yes, if we need to do operations on Rows, I prefer to keep these operations in only one place: RowsCollection.
This can be repetitive and annoying but this is something I don't want to see anymore in PrestaShop code: rely on a basic array to store Domain Collection of models. We need to see explicit intent everywhere and limit the return of multiple types like "string, bool, int, null or mysql_resource" you can find in some dark places of PrestaShop code.

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 3, 2018

Contributor

Note that I'm totaly open to an abstract class "Collection" and make every collection of PrestaShop extends it to make this work "less annoying".

private $name;
/**
* @var string Unique grid idetifier

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Apr 25, 2018

Contributor
- idetifier
+ identifier
/**
* @var array|Column[]
*/
private $columns = [];

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Apr 25, 2018

Contributor

same here, I'd like to see a ColumnCollection object: if we want to do operations on collections (like re-order), it's easier to have a specific object for that. You can re-adapt my own.

This comment has been minimized.

@sarjon

sarjon Apr 27, 2018

Member

done. 👍

public function addRowAction(RowAction $rowAction)
{
if (isset($this->rowActions[$rowAction->getIdentifier()])) {
throw new NonUniqueRowActionException(sprintf('Row action "%s" already exsists on grid definition'));

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Apr 25, 2018

Contributor

this means you can't override a default column: I think it's the intended behavior.

Minor typo: exsists/exists

This comment has been minimized.

@sarjon

sarjon Apr 27, 2018

Member

you are right, it should be able to replace actions.

@@ -24,9 +24,8 @@
* International Registered Trademark & Property of PrestaShop SA
*/
namespace PrestaShop\PrestaShop\Core\Grid\Action;
namespace PrestaShop\PrestaShop\Core\Grid\Column;

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Apr 26, 2018

Contributor

wow I forgot that ^^

@@ -26,54 +26,52 @@

This comment has been minimized.

@mickaelandrieu
'layoutHeaderToolbarBtn' => [],
'layoutTitle' => $this->get('translator')->trans('Logs', array(), 'Admin.Navigation.Menu'),
'layoutTitle' => $this->get('translator')->trans('Logs', [], 'Admin.Navigation.Menu'),

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Apr 26, 2018

Contributor

you can use $this->trans('Logs', 'Admin.Navigation.Menu')

/**
* Trait TranslatorAwareTrait is used for services that depends on translator
*/
trait TranslatorAwareTrait

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Apr 26, 2018

Contributor

I'm not fan of traits, let me think about it... wdyt @eternoendless ?

This comment has been minimized.

@sarjon

sarjon Apr 26, 2018

Member

if you think it's not worth having trait for translator, it's fine for me. However, are there some drawbacks with traits?

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Apr 26, 2018

Contributor

Let's keep it as it, for now, we have 3 or 4 months before 1.7.5 to see if it's a good or bad idea.

Personally, I prefer to inject required dependencies in the constructor.

* @todo: logs do not have row actions, these are defined for testing purpose and will be removed
*/
protected function getRowActions()
{

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Apr 26, 2018

Contributor

this function should return a RowActionCollection: am I right?

{
// make search parameters compatible with logRepositoryQuery

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Apr 26, 2018

Contributor

each page can have specific default values, this is why I've created a LogFilters class in my pull request on this specific subject. I can make these classes implement your interface: can you review my pr? Tomorrow we can talk about how we could improve the 2 prs to make them compatible :)

In an ideal world, we merge this one first. Then I rebase mine and I adapt it to keep the Filters instance part of every controller action arguments.

Then I work on the storage/delete of saved parameters and we are good for every listing page... except security but we will be iso with 1.6: it's good enough to be merged.

This comment has been minimized.

@sarjon

sarjon Apr 26, 2018

Member

Yes, this search part is really tricky at the moment. There is also duplicate for default filters. For example, each Grid definition provides default 'order by' and 'order way' but the same defaults are in LogsFilters, I guess we have to remove it from grid definition?

*/
public function createFromRequest(Request $request, GridDefinitionInterface $definition)
{
$limit = $request->query->get('limit', 10);

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 2, 2018

Contributor

the default values can be different by page, look at Product page and you'll see others values :)

*/
public function getRows($filters = null)
{
// make search parameters compatible with logRepository query

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 2, 2018

Contributor

note that this work is not part of this pull request, see #8984 for search filters management system

* @param string $identifier Grid identifier should be unique per grid and will act as ID on html table element
* @param string $name Grid name
* @param array $columnViews Grid columns
* @param FormView $filterForm Filters form view

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 2, 2018

Contributor

please rename your variable $formView

}
$form = $formBuilder
->setData([]) //@todo: filters data should be

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 2, 2018

Contributor

duly noted, after merging this contribution I'll rebase mine and I will complete.

@mickaelandrieu mickaelandrieu requested a review from eternoendless May 3, 2018

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

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 3, 2018

Contributor

Ok for this interface, I'll adapt my pull request once this once is merged.

@@ -60,7 +60,7 @@ twig:
'%admin_page%/Configure/AdvancedParameters': AdvancedParameters
'%admin_page%/Configure/ShopParameters': ShopParameters
globals:
webpack_server: false
webpack_server: true

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 3, 2018

Contributor

don't forget to set to false again :)

}
/**
* @return string|null

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 3, 2018

Contributor

this is wrong, by default the value of icon is a string (' '), and this is good :-)

}
/**
* @return string|null

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 3, 2018

Contributor

same here, icon should never be null but empty string

public function getRowsTotal();
/**
* Get query which is used to retireve grid rows from data source

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 3, 2018

Contributor
- retireve
+ retrieve
*/
public function getQuery(SearchCriteriaInterface $searchCriteria)
{
$logQuery = $this->logRepository->getAllWithEmployeeInformationQuery([

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 3, 2018

Contributor

Note here: this is something I don't have solved yet: how to filter on new entities? For instance, if we add a new column to Product table that is not owned by the Product table?

In legacy part, we have what we call "filter_key' => https://github.com/PrestaShop/PrestaShop/pull/9004/files

We will implement it at some point, but this won't change the interfaces, only the implementations 👍

This comment has been minimized.

@sarjon

sarjon May 3, 2018

Member

You mean if new column is added by some module for example?

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 3, 2018

Contributor

yes, this is one of the main needs of PrestaShop developers in Grid, isn't it? Be able to add/remove a column where the data that comes from a different SQL table, and place it in the right position.

This comment has been minimized.

@sarjon

sarjon May 3, 2018

Member

i think it can be done already. Developers can add custom column to grid definition and they also can modify query builder to select/join more fields.

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 3, 2018

Contributor

great! I had the feeling we could do it too, but I prefer to ask :)

*/
public function getQuery(SearchCriteriaInterface $searchCriteria)
{
$logQuery = $this->logRepository->getAllWithEmployeeInformationQuery([

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 3, 2018

Contributor

for what it means, $logQuery returns a QueryBuilder: how about naming it $logQueryBuilder?

]);
$this->hookDispatcher->dispatchForParameters('modifyLogGridQuery', [
'query' => $logQuery,

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 3, 2018

Contributor

this is query_builder and modifyLogGridQueryBuilder, note this class is a good candidate to be refactored later :)

],
];
$columns = new ColumnCollection();

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 3, 2018

Contributor

how about ColumnCollection:fromArray function?

$columns = new ColumnCollection();
$position = 0;
foreach ($columnsArray as $columnArray) {

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 3, 2018

Contributor

"this" is what I want to avoid with Collection classes :)

$column = Column::fromArray($columnArray);
$columns->add($column);
$position += 2;

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 3, 2018

Contributor

why do you need to make position of columns "even"?

This comment has been minimized.

@sarjon

sarjon May 3, 2018

Member

probably we don't, i was playing with testing module and it's really easy to add new column with position 3, so it will be between column with position 2 and column with position 4.

however, i think better solution would be to create new method on ColumnCollection class something like addAfter($column, $identifier);

do you think it's better that way?

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 3, 2018

Contributor

yes :) I think that functions like addAfter or addBefore will be loved by PrestaShop developers but this may be done later! Let's keep the scope of this contribution reviewable.

],
];
$actions = new GridActionCollection();

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 3, 2018

Contributor

same here: create GridActionCollection::fromArray function.

'sortOrder' => 'desc',
'filters' => array()
));
// temporary search criteria class, to be removed

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 3, 2018

Contributor

👍 thanks :)

let showSqlActionId = commonActionSuffix + 'ps_show_query';
let exportSqlManagerActionId = commonActionSuffix + 'ps_export_sql_manager';
$gridPanel.on('click', refreshListActionId, () => this._onRefreshClick());

This comment has been minimized.

@sarjon

sarjon May 3, 2018

Member

could there be a better way to handle most common grid actions?

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 3, 2018

Contributor

I ... don't know. For now, let's make it works, then we can make it easy, then we can make it pretty :)

As it's noted as "private", we can redo it when needed.

The real issue is that I'm not sure the actions are always available...

$gridPanel.on('click', refreshListActionId, () => this._onRefreshClick());
$gridPanel.on('click', showSqlActionId, () => this._onShowSqlQueryClick());
$gridPanel.on('click', exportSqlManagerActionId, () => this._onExportSqlManagerClick());

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 3, 2018

Contributor

don't we have already a js module that handle SQLManager stuff?

This comment has been minimized.

@sarjon

sarjon May 3, 2018

Member

we do, with hardcoded catalog page classes, it makes hard to use it, in my opinion that SqlManager should be reworked to make it more flexible, what do you think?

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 3, 2018

Contributor

I think you're right 👍

@@ -32,7 +32,7 @@ framework:
session:
handler_id: ~
fragments: ~
http_method_override: true
http_method_override: false

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 5, 2018

Contributor

can you explain why do you need to change this value?

This comment has been minimized.

@sarjon

sarjon May 5, 2018

Member

nope, it doesn't need to be changed. Reverted.

@mickaelandrieu mickaelandrieu merged commit 3633394 into PrestaShop:develop Jun 28, 2018

1 of 3 checks passed

Codacy/PR Quality Review Not up to standards. This pull request quality could be better.
Details
code-review/reviewable 90 files, 67 discussions left (eternoendless, LouiseBonnard, mickaelandrieu, PierreRambaud, rokaszygmantas, sarjon)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Jun 28, 2018

This doesn't need QA approval as this introduce no "functional feature" available for e-merchants for now :)

Good job @sarjon, thanks everyone for review.

Now:

  • Enable search
  • Enable data management using hooks
  • Fix bugs and add tests
  • Update docs
  • and migrate 35 pages of Back Office the better and the faster we can :-)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment