Skip to content
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

Show permissions for all modules and all roles by default #10

Closed
olafgrabienski opened this issue Oct 25, 2022 · 9 comments · Fixed by #11
Closed

Show permissions for all modules and all roles by default #10

olafgrabienski opened this issue Oct 25, 2022 · 9 comments · Fixed by #11
Labels
enhancement New feature or request question Further information is requested

Comments

@olafgrabienski
Copy link
Member

With this module, the page admin/config/people/permissions doesn't show much information out of the box. I see the filters and I'm asked to »select at least one value from both the Roles and Modules select boxes above and then click the "Filter Permissions" button«. That's a lot! In my opinion it would be better to select 'All Roles' and 'All Modules' by default. What do others think about it?

@olafgrabienski olafgrabienski added the question Further information is requested label Oct 25, 2022
@yorkshire-pudding
Copy link
Collaborator

@olafgrabienski - this could be an improvement, though if you have more checkboxes than can be saved maybe not. I think for the majority of users, what you suggest would be an improvement.

I was wondering also whether it should be auto-submit so you don't need to press 'filter permissions' button but it just updates as selections are changed.

@olafgrabienski
Copy link
Member Author

Thanks for your feedback! I guess I didn't understand what you mean by "if you have more checkboxes than can be saved maybe not".

Btw, I tested the module coming from backdrop/backdrop-issues#5796, which asks to integrate Filter Permissions into Backdrop core. And I thought, if that happens, the default behavior should be more consistent with other filterable Backdrop pages.

@yorkshire-pudding yorkshire-pudding added the enhancement New feature or request label Oct 26, 2022
@yorkshire-pudding yorkshire-pudding changed the title Change default behavior? Show permissions for all modules and all roles by default Oct 26, 2022
@yorkshire-pudding
Copy link
Collaborator

I guess I didn't understand what you mean by "if you have more checkboxes than can be saved maybe not".

While one purpose of the module is pure usability, another is the ability to save permission forms if they have more checkboxes than can be saved. See the README:

Sites that have one or more of the following factors may struggle to save the permissions form:

  • a large number of modules that have permissions attached
  • a large number of content types
  • a large number of roles

While there are some functions within Backdrop that try to mitigate this, on some hosting it may not be possible to adjust (see the documentation page) and this module can enable saving of permissions, where it would not otherwise be possible to save.

Filter Permissions does this by only saving the visible permissions.

I have an idea how to mitigate this (PR on the way)

After further investigation, auto-submit won't work if the use case above is in play as the checkboxes would still be on screen.

@yorkshire-pudding
Copy link
Collaborator

yorkshire-pudding commented Oct 26, 2022

@olafgrabienski @bugfolder @stpaultim

I've added a PR to change the default filters to be "All roles" and "All modules". I've also added a test to see if the form has too many checkboxes to save; if it does, a warning is displayed and the save button is hidden.

This can be tested by reducing max_input_vars in the Backdrops .htaccess file

image

What do you think?

@olafgrabienski
Copy link
Member Author

@yorkshire-pudding Thanks for the helpful clarification, and for the PR! I like the approach shown in the screenshot (no time for testing at the moment), and I'm also curious what Robert and Tim think about it.

@yorkshire-pudding
Copy link
Collaborator

I discussed with @stpaultim during office hours 19 today and he supports the change. Let's see what @bugfolder thinks

@bugfolder
Copy link
Contributor

permissions

This warning alone is worth the price of admission. 👍🙏😄 And this module should be in core!

@bugfolder
Copy link
Contributor

Oh, for the record: WFM and LGTM.

@stpaultim
Copy link
Member

I discussed with @stpaultim during office hours 19 today and he supports the change.

Confirmed. I think is a great improvement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants