Admin Grid Mass action Select / Unselect All issue #9610

[minesh0111]

Admin Grid Mass Action Select All and Unselect All work only on primary_key of collection object no matter if we set other field as mass action field using grid method setMassactionIdField other than primary_key

Preconditions

1. Magento version 2.1.3

Steps to reproduce

1. Found this issue while creating custom module where i need mass action id field other than primary_key of collection.

Expected result

1. It should Select All and Unselect All on click of option
2. It should post MassactionIdField on submit

Actual result

1. Checkbox checked and unchecked not working on click of option
2. Always post primary_key value of collection on submit

[magento-cicd2]

All committers have signed the CLA.

[fooman]

hi @minesh0111 thanks for your contribution. Quick question with your new implementation wouldn't this result in an additional load of the ids in the case that the massaction is the primary key?

EDIT: disregard my comment - it looks like the clear() and different collection is already in the existing implementation.

[fooman]

There are a couple of unit test failures in relation to the changes you made as outlined here - would you mind taking a look to see if you can get them updated to the new code?

[madhav-primetraffic]

can someone guide me on The Travis CI build failed report and how to solve it ?

[minesh0111]

@fooman i am new to unit test can you guide me to fix that ?

[convenient]

Have a look at the failing test class and method

Magento\Backend\Test\Unit\Block\Widget\Grid\Massaction\ExtendedTest::testGetGridIdsJsonWithUseSelectAll

http://devdocs.magento.com/guides/v2.0/config-guide/cli/config-cli-subcommands-test.html

Tests can be hard to get your head around initially, but once you understand how phpunit is configured it really opens up doors and improves robustness of code.

You can see the directories specified for unit testing here: https://github.com/magento/magento2/blob/develop/dev/tests/unit/phpunit.xml.dist#L14

(I don't work for magento, this just popped up on my feed)

[minesh0111]

@convenient thanks for your feedback. yes i need to understand unit test first then will go for magento unit test.

do we need to create unit test our self or PHP unit test will do this job ?

[convenient]

There is already a test in place covering the area you changed I believe. Your changes tripped it up as they're a change in functionality which is now causing them to fail. You'll need to

• run these test locally to see them fail
• update the tests to make them pass
• push

[minesh0111]

@convenient do i need to make another pull request for same ?

[convenient]

You should push up into the same branch. All code changes to make your feature work, as well as going green in the unit tests, should be in this Pull Request. 👍

[minesh0111]

@convenient All checks have passed 👍

[fooman]

good work @minesh0111 and thanks @convenient for helping out.

@minesh0111 do you have a minimal example module that could be used to verify your changes?

[minesh0111]

@fooman if a separate module is required for testing then I'll need to create a new one, because I had integrated the code in the existing files and not in a new module.

Please let me know

[fooman]

@minesh0111 looks good - nothing further needed for now. We'll be running a few more tests internally on this and get back to you.

One question: do you see any reason why this change should not also be applied to
Magento\Backend\Block\Widget\Grid\Massaction\AbstractMassaction which has the same getGridIdsJson() implementation?

[minesh0111]

@fooman yes you are right. i will update for Magento\Backend\Block\Widget\Grid\Massaction\AbstractMassaction as well.

[minesh0111]

@fooman Done for Magento\Backend\Block\Widget\Grid\Massaction\AbstractMassaction

[fooman]

thanks @minesh0111 - the travis issue seems to have been some date issue. A rerun solved it. I'll progress your PR along and keep you posted.

[fooman]

@minesh0111 unfortunately bad news - there is another failure coming from this functional test https://github.com/magento/magento2/blob/develop/dev/tests/functional/tests/app/Magento/PageCache/Test/TestCase/CacheStatusOnScheduledIndexingTest.php with this patch in place - I have requested some more info and will post back here when I know more.

[minesh0111]

@fooman this commit going crazy now. can you point me on this. how can i test this functional test ?

[fooman]

@minesh0111 I understand the frustration. You unfortunately picked a component (grids) which is used all over Magento so we need to make sure that the introduced changes don't break anything.

@ishakhsuvarov do you mind providing some more guidance on what actually failed in the mentioned test?

[minesh0111]

@ishakhsuvarov, @fooman any updates on this commit ?

[fooman]

@minesh0111 Okay I found the culprit. With your changes applied when you head to System > Index Management the Select all, etc do not work any more.

The problem comes from here. So Magento sometimes seems to set the MassactionIdField on the parent grid block and sometimes directly on the massaction block.

One thing we could try is to change

    $massActionIdField = $this->getParentBlock()->getMassactionIdField();

to

    if ($this->getMassactionIdField()) {
        $massActionIdField = $this->getMassactionIdField();
    } else {
        $massActionIdField = $this->getParentBlock()->getMassactionIdField();
    }

and then we'll see how we go.

[minesh0111]

@fooman updated as suggested.

[fooman]

Thanks @minesh0111 for getting the build green again. I'll keep you posted.

[fooman]

Thanks @minesh0111 for persevering - your contribution has been merged.

[minesh0111]

Thanks @fooman, @magento-team, @convenient and all who helped me in this contribution