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

dev/core#4840 StandaloneUsers - Improve performance & usability of 'User Permissions' screen #31162

Merged
merged 5 commits into from
Sep 26, 2024

Conversation

colemanw
Copy link
Member

@colemanw colemanw commented Sep 24, 2024

Overview

Fixes https://lab.civicrm.org/dev/core/-/issues/4840

This implements a Drupal-like screen which shows all roles as table columns.
Implied permissions are indicated in grey.
Bulk-update actions are used instead of inline editing, which is more performant.

Screenshot 2024-09-24 at 6 36 59 PM

Technical Details

This modifies the signature of the RolePermission api to give pivot-table-like output, with one row per permission, and every role as a field.
This api was created solely for the purpose of this screen & that's the only use I can find. As Standalone is still in beta, I think it's a safe change to make at this point.

…an id

This brings crmSearchTaskUpdate in-line with crmSearchTaskApiBatch
which dynamically loads the primary_key for the entity.
Copy link

civibot bot commented Sep 24, 2024

🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷

Introduction for new contributors...
  • If this is your first PR, an admin will greenlight automated testing with the command ok to test or add to whitelist.
  • A series of tests will automatically run. You can see the results at the bottom of this page (if there are any problems, it will include a link to see what went wrong).
  • A demo site will be built where anyone can try out a version of CiviCRM that includes your changes.
  • If this process needs to be repeated, an admin will issue the command test this please to rerun tests and build a new demo site.
  • Before this PR can be merged, it needs to be reviewed. Please keep in mind that reviewers are volunteers, and their response time can vary from a few hours to a few weeks depending on their availability and their knowledge of this particular part of CiviCRM.
  • A great way to speed up this process is to "trade reviews" with someone - find an open PR that you feel able to review, and leave a comment like "I'm reviewing this now, could you please review mine?" (include a link to yours). You don't have to wait for a response to get started (and you don't have to stop at one!) the more you review, the faster this process goes for everyone 😄
  • To ensure that you are credited properly in the final release notes, please add yourself to contributor-key.yml
  • For more information about contributing, see CONTRIBUTING.md.
Quick links for reviewers...

➡️ Online demo of this PR 🔗

@civibot civibot bot added the master label Sep 24, 2024
Copy link

civibot bot commented Sep 24, 2024

The issue associated with the Pull Request can be viewed at https://lab.civicrm.org/dev/core/-/issues/4840

This adds an overrideable function to BasicSaveAction with the same name & signature as the BasicUpdateAction,
allowing both to be overridden by performance-minded api actions.
@ufundo ufundo added the run-standalone Civibot should setup demos+tests for Standalone label Sep 24, 2024
@colemanw
Copy link
Member Author

I believe the upgrade test failure is unrelated

…s' screen

This implements a more Drupal-like screen which shows all rows as table columns
Implied permissions are also shown (in grey).
Bulk-update actions are used instead of inline editing, which is more performant
Copy link
Contributor

@artfulrobot artfulrobot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really promising, thanks @colemanw

I'm not seeing the role-columns?

image

@colemanw
Copy link
Member Author

@artfulrobot I noticed the missing columns on demo too but as soon as I cleared caches they appear. I think it's just a timing issue with buildkit builds & demo sites but on real sites it ought to be fine.
Maybe we need to add a final managedEntity::reconcile as a post-install step for standalone to avoid that initial blank-ness prior to the first cache clear.

@artfulrobot
Copy link
Contributor

@colemanw I didn't think to clear caches bc it was a new build - but you're right, a cache clear fixed it. I've r-run and it works really well.

I think it's pretty darned wonderful 😄

I agree there's no need to worry re changing the API that was only for this screen.

Minor suggestion is to provide a title attribute to the various coloured ☑ icons. This would help visual users learn what it meant (and why they couldn't click it!) and would help screen reader users to access the information that is otherwise not present for them.

@artfulrobot
Copy link
Contributor

... now how long before someone uses the "select all" button rather than reading what the permissions do... 🦶 🔫 🤣

@colemanw
Copy link
Member Author

colemanw commented Sep 26, 2024

@artfulrobot maybe we could merge this as "better than it was" and then iterate on those other items.
I've got an idea about some kind of "tree" searchDisplay that would make the hierarchical nature of the permissions more visual...

Update: done! #31189

@artfulrobot artfulrobot merged commit 5950854 into civicrm:master Sep 26, 2024
1 of 2 checks passed
@colemanw colemanw deleted the userRolePermissions branch September 26, 2024 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has-test master run-standalone Civibot should setup demos+tests for Standalone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants