salesagility / SuiteCRM

SuiteCRM - Open source CRM for the world
https://www.suitecrm.com
GNU Affero General Public License v3.0
4.4k stars 2.06k forks source link

Listview record selection shouldn't be enabled if no mass actions are available #8316

Open marin-h opened 4 years ago

marin-h commented 4 years ago

Issue

Surely it qualifies more as a suggestion than a bug. It's possible to select records in any listview when no mass actions are available for the current user.

screenshot

Expected Behavior

If there are no mass actions available, don't allow the user to select multiple records.

Actual Behavior

User can select records with checkboxes even if there are no mass actions allowed.

Possible Fix

Setting $this->multiSelect in ListViewSmarty class to false if there are no mass actions allowed. We could add an if clause for setting this as false when there are no mass actions available for the user.

Applying this patch we can hide those checkboxes:

imagen

Steps to Reproduce

  1. Browse a Listview with a user with no massupdate or delete permissions.
  2. Click search button.
  3. Checkboxes and select actions are shown.
  4. If you select any record, no mass actions are available.

Context

Confusing UX when searching for records.

Your Environment

marin-h commented 4 years ago

Investigating this class I found this snippet, which I'm not sure what it's meant to do.

            if (count($action_menu['buttons']) > 0) {
                $this->ss->assign('actionDisabledLink', preg_replace("/id\s*\=(\"\w+\"|w+)/i", "", $action_menu['buttons'][0]));
            }

I think this condition is always true: count($action_menu['buttons']) > 0, because the array buttons always has values in it (if a button is not supposed to show, the value stored in this array is an empty string), then this smarty variable $actionDisabledLink always is assigned an empty string.

Then looking for this smarty variable actionDisabledLink in ListViewPagination.tpl and ListViewPaginationBottom.tpl and ListViewPaginationTop.tpl, I think has no effect, because it's value is an empty string in all cases, so the condition never evaluates to true:

                        { if $actionDisabledLink ne "" }<div class='selectActionsDisabled' id='select_actions_disabled_{$action_menu_location}'>{$actionDisabledLink}</div>{/if}

I am guessing that perhaps this logic was intended to hide/disable select action links dropdown or the checkboxes when no mass actions are allowed to show in the listview?

To achieve that we could probably do the following:

1) Change the condition to check if all the buttons are empty strings: 2) Set multiSelect class property to false if there are no buttons to show. 3) Drop the actionDisabledLink logic since it has no effect? This variable seems to have no sense, but maybe I am missing something here.

            if (count(array_filter($action_menu['buttons'], function ($e) {
                return $e != '';
            })) === 0) {
                $this->multiSelect = false;
            }
Mac-Rae commented 4 years ago

Hi @marin-h,

Thanks for the suggestion, I agree that it would make more sense to this was the case

marin-h commented 4 years ago

Great. Unfortunately today I came across that the solution I thought about breaks the ListView sorting actions, I don't know exactly why it kind of depends on those checkboxes. I'll try hiding them instead of not rendering them to see how it goes.