Skip to content

Commit

Permalink
Enhance usergroup permission handling and display (#16469)
Browse files Browse the repository at this point in the history
### What does it do?
Among a number of refactoring moves, the solution submitted here
provides a new and centralized method to update the “Permissions in
Selected Policy” component found in the various Create and Update
windows within the ACLs area. Also:

1. Created a new parent class for the 5 User Group permissions grids
2. Created a new parent class for the 5 Create windows
3. Refactors implemented trim the code overall by ~450 lines
4. Because the changes were so significant, code style issues were fixed
at the same time (instead of in a separate commit)
5. Updated style of the permissions display, both in windows and in the
row expander (within the grids), making them significantly more readable
6. Added a handful of Lexicons for select windows to make all window
headings consistent. Additionally, a couple of descriptions were missing
or needed updating.
7. Altered how the window combos are set to avoid issue where certain
combos would initially show the raw value instead of the displayField
value (name in most cases)

#### Special Note
In the editing windows, I changed the Context combo’s displayField to
`name` instead of `key` (`context_key`). Even as a developer, I've
always found showing the key cumbersome — to me it's much more natural
to associate with the name of any object rather than its id/key. If
reviewers and code owners agree, I'd also implement that in the grids
(which currently still display the key).

### Why is it needed?
As mentioned in #16386, permissions lists (in windows) were not updating
correctly. Also:

1. Permissions were not initially showing up in the edit window (see
Figure 1)
2. A number of UI display issues needed solving (items 5–7 above) (see
Figures 2 & 3)
3. Lastly, this are had a large amount of duplicate/very similar code,
making this area far from DRY.

Figure 1 — Before fix, missing perms list
<img width="450" alt="fig-1"
src="https://github.com/modxcms/revolution/assets/689075/28b6fc61-8d74-46c4-ac1d-b7ebce8018fe">

Figure 2 — Window before and after, showing css update
<img width="919" alt="fig-1"
src="https://github.com/modxcms/revolution/assets/689075/923ada3e-4ff9-4672-8579-1a5e4068fe89">

Figure 3 — Grid before and after, showing css update
<img width="1176" alt="fig-3"
src="https://github.com/modxcms/revolution/assets/689075/14ef0f81-53a7-49fb-816f-d4c4591b3401">

Figure 4 — Video clip showing correct permissions display when opening
and after closing and re-opening editing windows

https://github.com/modxcms/revolution/assets/689075/f008cd55-97fb-432a-aa7a-d2bf8c2b28df


### How to test

1. Run grunt build to ensure css is updated.
4. Ensure you have a number of ACLs created for each of the Permissions
areas in a User Group (Context, Resource Group, Category, Namespace, and
Source).
5. Test creating and editing various entries to verify the editing
windows always display the correct information.

### Related issue(s)/PR(s)
Resolves #16386

---------

Co-authored-by: Jim Graham <info@sparkmediagroup.com>
Co-authored-by: Jason Coward <jason@opengeek.com>
  • Loading branch information
3 people authored Jul 5, 2024
1 parent 69c7efb commit 7d90842
Show file tree
Hide file tree
Showing 14 changed files with 1,198 additions and 1,559 deletions.
88 changes: 88 additions & 0 deletions _build/templates/default/sass/index.scss
Original file line number Diff line number Diff line change
Expand Up @@ -1763,6 +1763,94 @@ iframe[classname="x-hidden"] {
border: 0 !important;
}

.info-list {
&.hide-list {
display: none;
}
.header {
color: scale-color($color: $coreFieldLabelColor, $lightness: 10%);
font: $fontMedium;
font-weight: 600;
&::before {
@include awesome-font;
content: fa-content($fa-var-unlock);
color: scale-color($color: $colorSplash, $saturation: -40%, $lightness: 25%);
display: inline-block;
margin: 0 .25rem;
}
span {
font-weight: normal;
}
}
ul {
margin-top: .5rem;
padding: .5rem;
border: 1px dashed $borderColor;
border-radius: $borderRadius;
li {
font: $fontMedium;
color: scale-color($color: $coreFieldLabelColor, $lightness: 15%);
padding: .15rem 0;
line-height: 1;
word-break: break-word;
}
}
@include grid-media($gtMobile) {
ul {
columns: 2;
column-gap: 2rem;
}
}
.x-window & {
margin-top: 1rem;
}
.x-grid3 & {
ul {
position: relative;
padding-left: 1rem;
background-color: #fcfbfb;
.x-grid3-row-alt & {
background-color: #f9faff;
}
&::before {
@include awesome-font;
content: fa-content($fa-var-unlock);
color: scale-color($color: $colorSplash, $saturation: -40%, $lightness: 25%);
position: absolute;
text-align: center;
font-size: 11px;
left: -7px;
top: -7px;
width: 14px;
height: 14px;
padding-top: 2px;
box-shadow: -1px 1px 3px #888;
border-radius: 100%;
background-color: #fff;
}
li {
font-style: italic;
}
}
@include grid-media($gtTabletP) {
ul {
columns: 3;
}
}
@include grid-media($gtTabletL) {
ul {
columns: 4;
}
}
@include grid-media($gtCinema) {
ul {
columns: 5;
}
}
}

}

/* for selectability in ext grids */
.x-selectable,
.x-selectable * {
Expand Down
26 changes: 22 additions & 4 deletions core/lexicon/en/access.inc.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
* @subpackage lexicon
*/
$_lang['access_category_management_msg'] = 'Manage User Group member access to Elements via Categories and optionally apply access policies.';
$_lang['access_category_create'] = 'Add Category Access';
$_lang['access_category_err_ae'] = 'An ACL for that Category already exists!';
$_lang['access_category_err_nf'] = 'Category ACL not found.';
$_lang['access_category_err_ns'] = 'Category ACL not specified.';
Expand All @@ -15,6 +16,7 @@
$_lang['access_category_update'] = 'Edit Category Access';
$_lang['access_confirm_remove'] = 'Are you sure you want to delete this security access control record?';
$_lang['access_context_management_msg'] = 'Manage User Group member access to Contexts and optionally apply access policies.';
$_lang['access_context_create'] = 'Add Context Access';
$_lang['access_context_err_ae'] = 'An ACL for that Context already exists!';
$_lang['access_context_err_nf'] = 'Context ACL not found.';
$_lang['access_context_err_ns'] = 'Context ACL not specified.';
Expand All @@ -28,6 +30,9 @@
$_lang['access_err_save'] = 'Error saving ACL!';
$_lang['access_grid_empty'] = 'No ACLs to display';
$_lang['access_grid_paginate'] = 'Displaying ACLs {0} - {1} of {2}';
$_lang['access_namespace_create'] = 'Add Namespace Access';
$_lang['access_namespace_remove'] = 'Delete Namespace Access';
$_lang['access_namespace_update'] = 'Edit Namespace Access';
$_lang['access_permissions'] = 'Access Permissions';
$_lang['access_permissions_add_document_group'] = 'Create a new document group';
$_lang['access_permissions_add_user_group'] = 'Create a new user group';
Expand All @@ -53,12 +58,14 @@
$_lang['access_policy_grid_empty'] = 'No policies to display';
$_lang['access_policy_grid_paginate'] = 'Displaying policies {0} - {1} of {2}';
$_lang['access_resourcegroup_management_msg'] = 'Manage User Group member access to Resource Groups and optionally apply access policies.';
$_lang['access_resourcegroup_create'] = 'Add Resource Group Access';
$_lang['access_resourcegroup_remove'] = 'Delete Resource Group Access';
$_lang['access_resourcegroup_update'] = 'Edit Resource Group Access';
$_lang['access_rgroup_err_ae'] = 'An ACL for that Resource Group already exists!';
$_lang['access_rgroup_err_nf'] = 'Resource Group ACL not found.';
$_lang['access_rgroup_err_ns'] = 'Resource Group ACL not specified.';
$_lang['access_rgroup_err_remove'] = 'An error occurred while trying to delete the Resource Group ACL.';
$_lang['access_rgroup_remove'] = 'Delete Resource Group Access';
$_lang['access_rgroup_update'] = 'Edit Resource Group Access';
$_lang['access_source_create'] = 'Add Media Source Access';
$_lang['access_source_err_ae'] = 'An ACL for that Media Source already exists.';
$_lang['access_source_remove'] = 'Delete Media Source Access';
$_lang['access_source_update'] = 'Edit Media Source Access';
Expand Down Expand Up @@ -108,8 +115,7 @@
$_lang['roles_msg'] = 'A role is, by definition, a position or status one holds within a certain situation. They can be used to group Users into a position or status within a User Group. Roles in MODX also have what is called "Authority". This is a number value that can be any valid integer. Authority levels are "inheritable downward", in the sense that a Role with Authority 1 will inherit any and all Group Policies assigned to itself, and to any Roles with higher Authority level than 1.';
$_lang['source_add'] = 'Add Media Source';
$_lang['namespace_add'] = 'Add Namespace';
$_lang['access_namespace_update'] = 'Edit Namespace Access';
$_lang['access_namespace_remove'] = 'Delete Namespace Access';

$_lang['filter_by_namespace'] = 'Filter by Namespace...';

$_lang['user_group_aw'] = 'Access Wizard';
Expand All @@ -135,13 +141,15 @@
$_lang['user_group_category_err_ae'] = 'User Group already has access to that Category.';
$_lang['user_group_category_policy_desc'] = 'The Policy to apply to this Context with Elements in the Category for this User Group. This will grant all Users in this User Group with the selected minimum Role all the Permissions in the Policy.';
$_lang['user_group_category_remove_confirm'] = 'Are you sure you want to delete this Category from this User Group?';

$_lang['user_group_context_access'] = 'Contexts';
$_lang['user_group_context_access_msg'] = 'Set the Contexts this User Group can access.';
$_lang['user_group_context_authority_desc'] = 'The minimum Role that will have access to the Permissions in the selected Policy for this context. Roles with stronger Authority (lower numbers) will inherit this access as well. Most situations can leave this at "Member".';
$_lang['user_group_context_context_desc'] = 'The Context to grant access to.';
$_lang['user_group_context_policy_desc'] = 'The Policy to apply to this Context for this User Group. This will grant all Users in this User Group with the selected minimum Role all the Permissions in the Policy.';
$_lang['user_group_context_err_ae'] = 'User Group already has access to that context.';
$_lang['user_group_context_remove_confirm'] = 'Are you sure you want to delete this Context from this User Group?';

$_lang['user_group_resourcegroup_access'] = 'Resource Groups';
$_lang['user_group_resourcegroup_access_msg'] = 'Set the Resource Groups this User Group can access.';
$_lang['user_group_resourcegroup_authority_desc'] = 'The minimum Role that will have access to the Permissions in the selected Policy for this context. Roles with stronger Authority (lower numbers) will inherit this access as well. Most situations can leave this at "Member".';
Expand All @@ -150,6 +158,7 @@
$_lang['user_group_resourcegroup_policy_desc'] = 'The Policy to apply to this Context with Resources in the Resource Group for this User Group. This will grant all Users in this User Group with the selected minimum Role all the Permissions in the Policy.';
$_lang['user_group_resourcegroup_remove_confirm'] = 'Are you sure you want to delete this Resource Group from this User Group?';
$_lang['user_group_resourcegroup_resource_group_desc'] = 'The Resource Group to grant access to.';

$_lang['user_group_source_access'] = 'Media Sources';
$_lang['user_group_source_access_msg'] = 'Set the Media Sources this User Group can access.';
$_lang['user_group_source_authority_desc'] = 'The minimum Role that will have access to the Permissions in the selected Policy. Roles with stronger Authority (lower numbers) will inherit this access as well. Most situations can leave this at "Member".';
Expand All @@ -158,5 +167,14 @@
$_lang['user_group_source_remove_confirm'] = 'Are you sure you want to delete this Media Source from this User Group?';
$_lang['user_group_source_source_desc'] = 'The Media Source to grant access to.';
$_lang['user_group_user_access_msg'] = 'Select which users you want in this User Group.';

$_lang['user_group_namespace_access'] = 'Namespaces';
$_lang['user_group_namespace_access_desc'] = 'Set the Namespaces this User Group can access.';

$_lang['user_group_namespace_namespace_desc'] = 'The Namespace to grant access to.';
$_lang['user_group_namespace_authority_desc'] = 'The minimum Role that will have access to the Permissions in the selected Policy. Roles with stronger Authority (lower numbers) will inherit this access as well. Most situations can leave this at "Member".';
$_lang['user_group_namespace_policy_desc'] = 'The Policy to apply to this Namespace for this User Group. This will grant all Users in this User Group with the selected minimum Role all the Permissions in the Policy.';

// Renamed, deprecated as of 3.0.4, remove in 3.1.0
$_lang['access_rgroup_remove'] = $lang['access_resourcegroup_remove'];
$_lang['access_rgroup_update'] = $lang['access_resourcegroup_update'];
4 changes: 3 additions & 1 deletion core/lexicon/en/user.inc.php
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@
$_lang['role_remove_confirm'] = 'Are you sure you want to delete this role?';
$_lang['roles'] = 'Roles';
$_lang['rrg_drag'] = 'Drag resources into resource groups here.';
$_lang['ugc_mutate'] = 'User Group Access to Context';
$_lang['ugc_grid_title'] = 'User Group Access to Contexts';
$_lang['ugc_remove'] = 'Delete User Group Access to this Context';
$_lang['ugrg_grid_title'] = 'User Group Access to Resource Groups';
Expand Down Expand Up @@ -196,3 +195,6 @@
$_lang['users'] = 'Users';
$_lang['user_createdon'] = 'Created On';
$_lang['user_createdon_desc'] = 'The date the user was created.';

// Renamed and/or deprecated as of 3.0.4; remove in 3.1.0
$_lang['ugc_mutate'] = 'User Group Access to Context'; // now in access.inc.php, access_context_create
2 changes: 1 addition & 1 deletion manager/assets/modext/modx.jsgrps-min.js

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions manager/assets/modext/widgets/core/modx.window.js
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,7 @@ MODx.Window = function(config) {
success: true
,failure: true
,beforeSubmit: true
,updateWindow: false
});
this._loadForm();
this.on('show',function() {
Expand Down
Loading

0 comments on commit 7d90842

Please sign in to comment.