Skip to content
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 Sell > Catalog > Monitoring listing action #13529

Merged
merged 27 commits into from Jul 12, 2019

Conversation

@zuk3975
Copy link
Contributor

commented Apr 23, 2019

Questions Answers
Branch? develop
Description? Migrate Sell > Catalog > Monitoring lists. Implemented 7 lists with working pagination, filtering and sorting. Lists actions should be added in another PR after ProductsController migration (lists requires product toggling, edit, delete actions and redirection to product list by specified category).
Type? refacto
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? #10552
How to test? use admin-dev/index.php/sell/catalog/monitoring to see the monitoring lists. The page should contain 7 lists according to legacy page. Each list should be filterable, sortable, paginatable independently from each other. The only action currently working is categories status toggling. All other actions should be implemented in another PR.

This change is Reviewable

@zuk3975 zuk3975 requested a review from PrestaShop/prestashop-core-developers as a code owner Apr 23, 2019

@matks matks referenced this pull request Apr 23, 2019
1 of 32 tasks complete

@zuk3975 zuk3975 changed the title [WIP] Migrate Sell > Catalog > Monitoring listing action Migrate Sell > Catalog > Monitoring listing action Apr 25, 2019

$gridId = ProductWithCombinationGridDefinitionFactory::GRID_ID;
}
if ($request->request->has(ProductWithoutCombinationGridDefinitionFactory::GRID_ID)) {

This comment has been minimized.

Copy link
@rokaszygmantas

rokaszygmantas Apr 25, 2019

Contributor

shouldn't elseif's be used in this case? You don't want to check further conditions when the first one already succeeded.

This comment has been minimized.

Copy link
@sarjon

sarjon Apr 25, 2019

Member

You could define some kind of map here. E.g.:

$map = [
  ProductWithoutCombinationGridDefinitionFactory::GRID_ID => [
    'grid_id' => ProductWithoutCombinationGridDefinitionFactory::GRID_ID,
    'grid_definition' => 'prestashop.core.grid.definition.factory.monitoring.product_without_combination',
  ],
  // ...    
];

This way it's easier to extend, then all you have to do is loop through your map until you find required item and return it right away.

$gridId = ProductWithCombinationGridDefinitionFactory::GRID_ID;
}
if ($request->request->has(ProductWithoutCombinationGridDefinitionFactory::GRID_ID)) {

This comment has been minimized.

Copy link
@sarjon

sarjon Apr 25, 2019

Member

You could define some kind of map here. E.g.:

$map = [
  ProductWithoutCombinationGridDefinitionFactory::GRID_ID => [
    'grid_id' => ProductWithoutCombinationGridDefinitionFactory::GRID_ID,
    'grid_definition' => 'prestashop.core.grid.definition.factory.monitoring.product_without_combination',
  ],
  // ...    
];

This way it's easier to extend, then all you have to do is loop through your map until you find required item and return it right away.

_controller: 'PrestaShopBundle:Admin/Sell/Catalog/Monitoring:index'
_legacy_controller: AdminTracking

admin_monitoring__search:

This comment has been minimized.

Copy link
@sarjon

sarjon Apr 25, 2019

Member

Seems like double _.

@zuk3975

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2019

@matks ready for review

@matks matks added the migration label Apr 26, 2019

@LouiseBonnard

This comment has been minimized.

Copy link
Contributor

commented May 2, 2019

Hello there, just two feedback I want to share about this page:

  • it seems the "+" button on the top right-hand corner of the frames does not work properly
  • it could be interesting to allow the users to add their own lists inside this monitoring page
@zuk3975

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2019

Hello there, just two feedback I want to share about this page:

  • it seems the "+" button on the top right-hand corner of the frames does not work properly
  • it could be interesting to allow the users to add their own lists inside this monitoring page

I guess you are talking about this: #11482.
The '+' is removed in migrated page as suggested.

@matks matks added the waiting for QA label Jul 9, 2019

@sarahdib

This comment has been minimized.

Copy link

commented Jul 10, 2019

Hello @zuk3975
List of empty categories : is OK

List of products with combinations but without available quantities for sale : is OK

List of products without combinations and without available quantities for sale : There is an error -> All product in migrated page have combinations and quantities.
Migrated page
image

Legacy page
image

List of disabled products : There is an error -> The product status is not on legacy page
Migrated page
image

Legacy page
image

List of products without images : is OK
List of products without description : is OK
List of products without price : is OK

@zuk3975

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2019

@sarahdib fixed the issues:

  • List of products without combinations and without available quantities for sale : There is an error -> All product in migrated page have combinations and quantities.

  • List of disabled products : There is an error -> The product status is not on legacy page

@sarahdib

This comment has been minimized.

Copy link

commented Jul 10, 2019

@zuk3975 thank you there is a conflict can you resolve it ?

I'm waiting for @matks review

@zuk3975 zuk3975 force-pushed the zuk3975:m/list/monitoring branch from 0e4f577 to f7c266b Jul 10, 2019

@matks

matks approved these changes Jul 11, 2019

@sarahdib sarahdib added QA ✔️ and removed waiting for QA labels Jul 12, 2019

@sarahdib sarahdib added this to the 1.7.7.0 milestone Jul 12, 2019

@sarahdib

This comment has been minimized.

Copy link

commented Jul 12, 2019

Thank you @matks for the review and @zuk3975 for the fix. All seems good to me :)

@matks

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2019

Except Travis 😭 I restart it ...

@matks

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2019

Thank you @sarahdib and @zuk3975

@matks matks merged commit 5af2201 into PrestaShop:develop Jul 12, 2019

2 checks passed

PrettyCI Code formatting
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

mbadrani added a commit to mbadrani/PrestaShop that referenced this pull request Jul 18, 2019

Merge pull request PrestaShop#13529 from zuk3975/m/list/monitoring
Migrate Sell > Catalog > Monitoring listing action
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.