marellocommerce / marello

Marello
Other
52 stars 18 forks source link

Notice: Undefined index: selectAll in RowSelectionSelectAllListener #34

Closed mkilmanas closed 6 years ago

mkilmanas commented 6 years ago

After upgrading to v1.4, we had issues with undefined index notice (which converts to Symfony exception and stops rendering).

The problem is on Marello\Bundle\DataGridBundle\EventListener\Datagrid\RowSelectionSelectAllListener lines 132-133

The condition does not look correct to me - the !empty($rowSelectionConfig['selectAll']) || (bool)$rowSelectionConfig['selectAll'] part when $rowSelectionConfig['selectAll'] is actually empty, evaluates the first part to false and this means it continues to evaluate next part - which actually uses the value that we have just established to be empty.

I don't fully understand the underlying intention to be able to submit a correction, but to me it seems that the last part is not needed at all because any value that evaluates as boolean true would already be caught in the !empty(...) part as any truthy value is considered non-empty.

Please let me know if I should submit a PR with the last condition removed or if there is a different solution to this.

24198 commented 6 years ago

Hi @mkilmanas,

Thank you for reporting! I will have a look at it as I haven't seen the issue for myself when testing the grids. Will take a look at it and get back to you.

Again thank you for reporting.

Kind Regards,

Jaimy Casteleijn

24198 commented 6 years ago

Hi @mkilmanas,

I was able to reproduce the issue and there was a two part on this one. One was the actual check in the Marello\Bundle\DataGridBundle\EventListener\Datagrid\RowSelectionSelectAllListener and another was in the Marello\Bundle\DataGridBundle\Resources\public\js/datagrid\listener\select-all-listener.js. The latter didn't look at the 'selectAll' option from the config which was making other grids going a little overboard by either not showing results or adding a non working version of the check-box selection. The issue has been resolved in https://github.com/marellocommerce/marello/releases/tag/1.4.1. Thank you for reporting!

Kind Regards,

Jaimy Casteleijn