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

[Admin] Mass deletion on admin grid (fixes #93) #8491

Merged
merged 13 commits into from Oct 30, 2017

Conversation

Projects
None yet
7 participants
@stefandoorn
Contributor

stefandoorn commented Sep 3, 2017

Q A
Bug fix? no
New feature? yes
BC breaks? probably not
Related tickets fixes #93
License MIT
  • When enabled, on the grid between the filters and the result table, an additional element is rendered with mass action buttons. In this case only delete is available.
  • In the result table checkboxes are rendered which have the ID of the resource as value.
  • As soon as user clicks the mass delete button, the ID's are fetched from the checkboxes, confirmation popup is shown and if clicked through the new URL is called. ID's are sent with a DELETE method call.

Done:

  • Docs
  • Behat tests
  • Specs
  • Implement on all resources that have an 'item delete' in admin grid
  • Inside ResourceController both the multiple and the single events are dispatched

Example screenshot:
zrzut ekranu 2017-10-27 o 10 03 04

To do / discuss:

  • Currently let's say you can delete 10 products. If product 3 delete runs into an error, number 1 and 2 are deleted already. I couldn't find a way to wrap the deletion into a transaction without adjusting the dependencies of ResourceController.
  • Sylius core team review.

@stefandoorn stefandoorn changed the title from [POC] Mass deletion on admin grid to [POC][WIP][Admin] Mass deletion on admin grid Sep 3, 2017

@stefandoorn stefandoorn changed the title from [POC][WIP][Admin] Mass deletion on admin grid to [POC][Admin] Mass deletion on admin grid Sep 6, 2017

@stefandoorn

This comment has been minimized.

Show comment
Hide comment
@stefandoorn

stefandoorn Sep 6, 2017

Contributor

Could someone of the Sylius team help me with a review? I'm not sure if this is the good approach, but I also think it's a valuable feature for 1.0.

Contributor

stefandoorn commented Sep 6, 2017

Could someone of the Sylius team help me with a review? I'm not sure if this is the good approach, but I also think it's a valuable feature for 1.0.

@gabiudrescu

why massAction and not bulkAction?

I know its a matter of semantics, but I find bulk more appropriate. WDYT?

@stefandoorn

This comment has been minimized.

Show comment
Hide comment
@stefandoorn

stefandoorn Sep 11, 2017

Contributor

I'm fine with bulk. @CoderMaggie had that also as feedback.

Contributor

stefandoorn commented Sep 11, 2017

I'm fine with bulk. @CoderMaggie had that also as feedback.

@CoderMaggie

This comment has been minimized.

Show comment
Hide comment
@CoderMaggie

CoderMaggie Sep 11, 2017

Member

@gabiudrescu @stefandoorn I am strongly for "bulk action" :)

Member

CoderMaggie commented Sep 11, 2017

@gabiudrescu @stefandoorn I am strongly for "bulk action" :)

@gabiudrescu

This comment has been minimized.

Show comment
Hide comment
@gabiudrescu

gabiudrescu Sep 11, 2017

Contributor

one more question for @stefandoorn: if I select the first entry in the list and then while keeping shift on my keyboard I select the last entry, is the browser going to select all the entries in the list?

if yes, then great -> you won a beer.
if no, but you can / want / will implement it -> you can win a beer
else -> we'll negotiate this until you you pick the 2nd option

WDYT?

Contributor

gabiudrescu commented Sep 11, 2017

one more question for @stefandoorn: if I select the first entry in the list and then while keeping shift on my keyboard I select the last entry, is the browser going to select all the entries in the list?

if yes, then great -> you won a beer.
if no, but you can / want / will implement it -> you can win a beer
else -> we'll negotiate this until you you pick the 2nd option

WDYT?

@stefandoorn

This comment has been minimized.

Show comment
Hide comment
@stefandoorn

stefandoorn Sep 11, 2017

Contributor

Haven't tested ;-) It's just select boxes - didn't do anything else fancy to it. So I guess it doesn't work.. There is also no way yet to select ALL - not only just what you see on the screen. I'll keep it in mind ;-)

Contributor

stefandoorn commented Sep 11, 2017

Haven't tested ;-) It's just select boxes - didn't do anything else fancy to it. So I guess it doesn't work.. There is also no way yet to select ALL - not only just what you see on the screen. I'll keep it in mind ;-)

@pamil pamil added this to the 1.1 milestone Sep 20, 2017

@stefandoorn

This comment has been minimized.

Show comment
Hide comment
@stefandoorn

stefandoorn Oct 23, 2017

Contributor

@GSadee is helping on the implementation. Renaming to 'bulk' has been done already. More updates expected over the next days.

Contributor

stefandoorn commented Oct 23, 2017

@GSadee is helping on the implementation. Renaming to 'bulk' has been done already. More updates expected over the next days.

@stefandoorn

This comment has been minimized.

Show comment
Hide comment
@stefandoorn

stefandoorn Oct 23, 2017

Contributor

@GSadee WDYT about transforming bulkDeleteAction into bulkAction, requiring an action paramater telling which handler to load (via something like a bulkHandlerResolver?

It will make things more complicated in this phase, but might make it more easy to add additional bulk actions later on (e.g. mass transform, invoice creation, sending them to external system, etc.).

Contributor

stefandoorn commented Oct 23, 2017

@GSadee WDYT about transforming bulkDeleteAction into bulkAction, requiring an action paramater telling which handler to load (via something like a bulkHandlerResolver?

It will make things more complicated in this phase, but might make it more easy to add additional bulk actions later on (e.g. mass transform, invoice creation, sending them to external system, etc.).

@stefandoorn

This comment has been minimized.

Show comment
Hide comment
@stefandoorn

stefandoorn Oct 23, 2017

Contributor

@GSadee Might not be needed actually, as the configs allow to render own templates in which own routes can be called of course. First idea would mean introducing an extra dependency to ResourceController, which is not preferable. The current dependencies already suit us for delete, update & state machine transitions. Anything else can be achieved by registering own bulk action templates I think.

Contributor

stefandoorn commented Oct 23, 2017

@GSadee Might not be needed actually, as the configs allow to render own templates in which own routes can be called of course. First idea would mean introducing an extra dependency to ResourceController, which is not preferable. The current dependencies already suit us for delete, update & state machine transitions. Anything else can be achieved by registering own bulk action templates I think.

@stefandoorn stefandoorn changed the title from [POC][Admin] Mass deletion on admin grid to [WIP][Admin] Mass deletion on admin grid (fixes #93) Oct 24, 2017

@gabiudrescu

This comment has been minimized.

Show comment
Hide comment
@gabiudrescu

gabiudrescu Oct 24, 2017

Contributor

@stefandoorn regarding multiple select, I did a PoC with https://github.com/rmariuzzo/checkboxes.js. you can check it out here: gabiudrescu@bbfcc4b

it's a dirty PoC, I added the dependency through yarn, but didn't know how to include it in the compiled app.js. maybe you can share this knowledge. on my local host, I manually created this file (web/assets/shop/js/checkboxes.js) and included it in the template for javascripts.

let me know what you think.

Contributor

gabiudrescu commented Oct 24, 2017

@stefandoorn regarding multiple select, I did a PoC with https://github.com/rmariuzzo/checkboxes.js. you can check it out here: gabiudrescu@bbfcc4b

it's a dirty PoC, I added the dependency through yarn, but didn't know how to include it in the compiled app.js. maybe you can share this knowledge. on my local host, I manually created this file (web/assets/shop/js/checkboxes.js) and included it in the template for javascripts.

let me know what you think.

@stefandoorn

This comment has been minimized.

Show comment
Hide comment
@stefandoorn

stefandoorn Oct 24, 2017

Contributor

@gabiudrescu This allows to use shift button to select multiple?

We also need to think about the option to select ALL (unregardless of the pagination)..

Contributor

stefandoorn commented Oct 24, 2017

@gabiudrescu This allows to use shift button to select multiple?

We also need to think about the option to select ALL (unregardless of the pagination)..

@GSadee

This comment has been minimized.

Show comment
Hide comment
@GSadee

GSadee Oct 24, 2017

Member

I guess that in this PR we should focus on basic bulk delete action functionality with proper tests, and maybe in future add to that e.g. select all button or other js magic 😃

Member

GSadee commented Oct 24, 2017

I guess that in this PR we should focus on basic bulk delete action functionality with proper tests, and maybe in future add to that e.g. select all button or other js magic 😃

@stefandoorn

This comment has been minimized.

Show comment
Hide comment
@stefandoorn

stefandoorn Oct 24, 2017

Contributor

Sure @GSadee :) Will PR it afterwards if the base functionality works :))

Contributor

stefandoorn commented Oct 24, 2017

Sure @GSadee :) Will PR it afterwards if the base functionality works :))

@gabiudrescu

This comment has been minimized.

Show comment
Hide comment
@gabiudrescu

gabiudrescu Oct 25, 2017

Contributor

@stefandoorn yes, this is what my PoC is all about. doing it with a button that selects all items on the page is a matter of a small JS:


jQuery(function($) {
    $('#btn-check-all').on('click', function(e) {
        $('#table1').checkboxes('check');
        e.preventDefault();
    });
});

you do the PHP stuff, I'll help with the UI / JS.

Contributor

gabiudrescu commented Oct 25, 2017

@stefandoorn yes, this is what my PoC is all about. doing it with a button that selects all items on the page is a matter of a small JS:


jQuery(function($) {
    $('#btn-check-all').on('click', function(e) {
        $('#table1').checkboxes('check');
        e.preventDefault();
    });
});

you do the PHP stuff, I'll help with the UI / JS.

@stefandoorn

This comment has been minimized.

Show comment
Hide comment
@stefandoorn

stefandoorn Oct 25, 2017

Contributor

True, but what I meant earlier: how to do a bulk action on ALL items (e.g. on all 100 products in the database) while pagination only shows 10 on the current page? @GSadee Ideas on that one? Or should we PR that separate afterwards?

Contributor

stefandoorn commented Oct 25, 2017

True, but what I meant earlier: how to do a bulk action on ALL items (e.g. on all 100 products in the database) while pagination only shows 10 on the current page? @GSadee Ideas on that one? Or should we PR that separate afterwards?

@GSadee

This comment has been minimized.

Show comment
Hide comment
@GSadee

GSadee Oct 25, 2017

Member

IMO this will be a nice feature but definitely in a seperate PR, after adding the basic functionality of bulk delete action.

Member

GSadee commented Oct 25, 2017

IMO this will be a nice feature but definitely in a seperate PR, after adding the basic functionality of bulk delete action.

@gabiudrescu

This comment has been minimized.

Show comment
Hide comment
@gabiudrescu

gabiudrescu Oct 25, 2017

Contributor

@stefandoorn i find it as a bad idea to have a "delete all" button from the database. consider this:

  • you have 400k products and you press that button
  • it will kill your webserver to handle this amount of data to be processed

I think being able to multiple select the products currently displayed in the view it's enough.

as @GSadee says, let's ensure the PHP part reaches the codebase and we'll add more JS flavour later 😁

also, if you know how to add a new js dependency in sylius, I would be grateful if you could share this knowledge.

Contributor

gabiudrescu commented Oct 25, 2017

@stefandoorn i find it as a bad idea to have a "delete all" button from the database. consider this:

  • you have 400k products and you press that button
  • it will kill your webserver to handle this amount of data to be processed

I think being able to multiple select the products currently displayed in the view it's enough.

as @GSadee says, let's ensure the PHP part reaches the codebase and we'll add more JS flavour later 😁

also, if you know how to add a new js dependency in sylius, I would be grateful if you could share this knowledge.

@stefandoorn

This comment has been minimized.

Show comment
Hide comment
@stefandoorn

stefandoorn Oct 25, 2017

Contributor

Alright, I agree.

@gabiudrescu Load it via yarn/npm, make sure it got added in package.json & yarn.lock, then adjust Gulpfile.js in AdminBundle to include it in the build process :-)

Then run yarn run gulp from root and it should be in your generated JS file that is loaded in admin. Not 100% tested, from memory.

Contributor

stefandoorn commented Oct 25, 2017

Alright, I agree.

@gabiudrescu Load it via yarn/npm, make sure it got added in package.json & yarn.lock, then adjust Gulpfile.js in AdminBundle to include it in the build process :-)

Then run yarn run gulp from root and it should be in your generated JS file that is loaded in admin. Not 100% tested, from memory.

@stefandoorn

This comment has been minimized.

Show comment
Hide comment
@stefandoorn

stefandoorn Oct 27, 2017

Contributor

Thanks to the tremendous help of @GSadee the current PR should reflect a reviewable version :-)

Contributor

stefandoorn commented Oct 27, 2017

Thanks to the tremendous help of @GSadee the current PR should reflect a reviewable version :-)

@stefandoorn

This comment has been minimized.

Show comment
Hide comment
@stefandoorn

stefandoorn Oct 27, 2017

Contributor

Updated PR description.

Contributor

stefandoorn commented Oct 27, 2017

Updated PR description.

@stefandoorn stefandoorn changed the title from [WIP][Admin] Mass deletion on admin grid (fixes #93) to [Admin] Mass deletion on admin grid (fixes #93) Oct 27, 2017

@pamil pamil added Feature and removed New Feature labels Oct 27, 2017

@pjedrzejewski pjedrzejewski merged commit e243d37 into Sylius:master Oct 30, 2017

2 checks passed

Scrutinizer 1 new issues, 66 updated code elements
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pjedrzejewski

This comment has been minimized.

Show comment
Hide comment
@pjedrzejewski

pjedrzejewski Oct 30, 2017

Member

Thank you Stefan & Grzesiu! :) Amazing work. I'd only discuss if adding the public method to the grid render interface was a BC Break. I think yes, but let me hear your thoughts. /cc @pamil

Member

pjedrzejewski commented Oct 30, 2017

Thank you Stefan & Grzesiu! :) Amazing work. I'd only discuss if adding the public method to the grid render interface was a BC Break. I think yes, but let me hear your thoughts. /cc @pamil

@stefandoorn

This comment has been minimized.

Show comment
Hide comment
@stefandoorn

stefandoorn Oct 30, 2017

Contributor

Makes sense that that is BC. Not directly sure how to get around it though.

Contributor

stefandoorn commented Oct 30, 2017

Makes sense that that is BC. Not directly sure how to get around it though.

*
* @return mixed
*/
public function renderBulkAction(GridViewInterface $gridView, Action $bulkAction, $data = null);

This comment has been minimized.

@pamil

pamil Nov 7, 2017

Member

It's a BC break for sure, we need to revert these changes and think of a better, backwards compatibility solution.

@pamil

pamil Nov 7, 2017

Member

It's a BC break for sure, we need to revert these changes and think of a better, backwards compatibility solution.

@@ -491,7 +664,10 @@ function it_generates_routing_for_a_section(
'permission' => false,
],
];
$routeFactory->createRoute('/products/', $indexDefaults, [], [], '', [], ['GET'])->willReturn($indexRoute);
$routeFactory

This comment has been minimized.

@pamil

pamil Nov 7, 2017

Member

Let's not do this kind of coding standard changes in a feature PR, it makes the real changes less clear.

@pamil

pamil Nov 7, 2017

Member

Let's not do this kind of coding standard changes in a feature PR, it makes the real changes less clear.

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