Clicking header cell bypasses checks inside of isRowSelectable

[patorjk]

Say I wanted to limit the number of checked rows to 5. I could do that using isRowSelectable, and it works - until I check the checkbox header. Example:

https://codesandbox.io/s/muidatatables-custom-toolbar-qg0i9

Expected Behavior

Selecting the header would not bypass checks made inside of isRowSelectable.

Current Behavior

When the checkbox header is selected, it runs isRowSelectable for each row, but for each function call it uses the prevSelectedRows object from the previous state and doesn't factor in that a row may have been added. This means it can bypass limits/checks a developer has placed inside of isRowSelected.

The code should instead create a copy of the selectedRows object and update it after each isRowSelected call to add the row if it can be selected.

Steps to Reproduce (for bugs)

1. Open https://codesandbox.io/s/muidatatables-custom-toolbar-qg0i9
2. Try to select more than 5 rows, see that the limit is in place.
3. Toggle the header checkbox a few times and see that it allows you to select more than 5 rows.

Your Environment

Tech	Version
Material-UI	3.2.2
MUI-datatables	2.9.0
React	16.4
browser	Crome
etc

[gabrielliwerant]

Hey, thanks for creating this issue.

I actually did know about this, but I'm not clear on what the best way to handle it would be. I could see a case for keeping the functionality of the check all separate from the is-selectable nature of individual rows. It's a bit subtle, but it means that "select all" really becomes "select all things not prevented from being selected" and it's a distinction that would become lost or at least impossible to separate if we combine the two.

Maybe the select all feature should have its own callbacks and options? What do you think?

[patorjk]

Hrm, after thinking a little on this, I see two broad scenarios:

• A developer wants to prevent a row's selectablity based on the data or position of that row.
• A developer wants to prevent a row's selectablity based on if other rows are selected.

For the first option, the way it currently works is fine. For the second option, selecting all is basically undefined, because there is no "all". Instead of allowing a developer to change how Select All works, I think it may make more sense to allow them to hide that option from the table (a boolean hideToggleAllRowSelect or something like that). Looks like this might be as simple as a boolean option and updating this line: https://github.com/gregnb/mui-datatables/blob/master/src/components/TableSelectCell.js#L92

[gabrielliwerant]

An option to hide the select all feature is an elegant solution. I would accept that PR.

[zuck]

IMHO #882 doesn't fix the issue at all. There's still no way to set a limit to multiple selection count.

My opinion is that the multiple selection should at least call a callback passing the current selectedRows. This callback should return the list of allowed selectable rows.

We already have onRowsSelect which could be used for that but it doesn't handle the result value.

UPDATE

I found a workaround that seems to work:

const [selectedRows, setSelectedRows] = useState([])

const options = {
    selectableRows: 'multiple',
    rowsSelected: selectedRows,
    onRowsSelect: (rowsSelected, allRows) => {
      const selectable = allRows.slice(0, MAX_SELECTABLE_ROW_COUNT)
      setSelectedRows(selectable.map(row => row.dataIndex))
    },
    isRowSelectable: (dataIndex, selectedRows) => {
      const isSelected = selectedRows.data.find(item => item.dataIndex === dataIndex)
      return isSelected || selectedRows.data.length < MAX_SELECTABLE_ROW_COUNT
    }
}