Skip to content

Commit

Permalink
Fix grouping grid sorting in ACL grids (#16529)
Browse files Browse the repository at this point in the history 
### What does it do?
Adds new grouping logic to ACL GetList processors and config props in
the corresponding Ext grid classes.

### Why is it needed?
Aside from the Settings grid,* other grouping grids (such as the ACL
ones) neither group properly, nor sort within groups properly.

### How to test

1. Rebuild may be necessary and compress_js must be off. 
2. Create a variety of ACL entries so there are at least a few main
items to group (this is the Minimum Role, which is what all ACLs are
grouped by as a default [can be changed in the column menu]), and a few
entries within those groups (meaning you'll need multiple contexts,
namespaces, resource groups, media sources, and element categories
created ahead of time to assign these roles to).
3. Verify that items are grouped as expected.
4. Verify that sorting on each column works as expected.

### Related issue(s)/PR(s)
No related issue.

* The settings grid does not group correctly if the grouping field is
changed. That will be addressed in a different PR.

---------

Co-authored-by: Jim Graham <[email protected]>
Co-authored-by: Jason Coward <[email protected]>
  • Loading branch information
@smg6511 @opengeek
3 people authored Aug 29, 2024
1 parent 46d232e commit 23d8965
Show file tree
Hide file tree
Showing 15 changed files with 323 additions and 82 deletions.
57 changes: 37 additions & 20 deletions .eslintrc.js
View file
Original file line number Diff line number Diff line change
@@ -1,41 +1,58 @@
module.exports = {
env: {
browser: true,
es2021: true,
es2021: true
},
extends: [
'eslint:recommended',
'airbnb-base',
'airbnb-base'
],
globals: {
MODx: 'readonly',
Ext: 'readonly',
_: 'readonly',
_: 'readonly'
},
ignorePatterns: [
'manager/assets/modext/workspace/workspace.panel.js',
'manager/assets/ext3/**/*.js',
'manager/assets/fileapi/**/*.js',
'manager/assets/lib/**/*.js',
'manager/assets/modext/modx.jsgrps-min.js',

'setup/assets/js/ext-core.js',
'setup/assets/js/ext-core-debug.js',
],
overrides: [
'setup/assets/js/ext-core-debug.js'
],
overrides: [],
parserOptions: {
ecmaVersion: 'latest',
ecmaVersion: 'latest'
},
rules: {
// TODO Enable rules gradually
indent: 0,
quotes: ['error', 'single'],
semi: 0,
'space-before-function-paren': 0,
'comma-dangle': 0,
'prefer-arrow-callback': 0,
'space-before-blocks': 0,
'object-shorthand': 0,
},
}
'arrow-parens': ['error', 'as-needed'],
'comma-dangle': ['error', 'never'],
'consistent-return': 0,
curly: ['error', 'all'],
eqeqeq: ['error', 'smart'],
'func-names': ['warn', 'as-needed'],
indent: ['error', 4, {
VariableDeclarator: 'first',
SwitchCase: 1
}],
'max-len': ['warn', {
code: 140,
ignoreComments: true
}],
'no-continue': 'warn',
'no-new': 'warn',
'no-param-reassign': 'warn',
'no-plusplus': ['error', {
allowForLoopAfterthoughts: true
}],
'no-underscore-dangle': 'warn',
'no-unused-vars': ['error', { args: 'none' }],
'no-use-before-define': ['error', 'nofunc'],
'object-shorthand': ['error', 'consistent'],
'one-var': ['error', 'consecutive'],
'prefer-arrow-callback': 'warn',
'prefer-rest-params': 'warn',
'semi-style': ['warn', 'last'],
'space-before-function-paren': ['error', 'never']
}
};
44 changes: 44 additions & 0 deletions core/src/Revolution/Processors/Model/GetListProcessor.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,9 @@ public function initialize()
$this->setDefaultProperties([
'start' => 0,
'limit' => 20,
'isGroupingGrid' => false,
'groupBy' => null,
'groupDir' => 'ASC',
'sort' => $this->defaultSortField,
'dir' => $this->defaultSortDirection,
'combo' => false,
Expand Down Expand Up @@ -187,6 +190,47 @@ public function getSortClassKey()
return $this->classKey;
}

/**
* Adds additional sortby criteria for grouping grids when the column being
* sorted is different than the one being grouped. Grouping is handled internally by Ext JS,
* so we do not (and should not) use groupby criteria in the query.
*
* @param xPDOQuery $c A reference to the current query being built
* @param string $sortBy The data index of the selected sorting column
* @param string $groupBy The data index of the selected grouping column
* @param string $groupKey The grouping column's fully qualified SQL column name
* @return void
*/
public function setGroupSort(xPDOQuery &$c, string $sortBy, string $groupBy, string $groupKey)
{
/*
When group sort and column sort are the same data index, sort the groups
based on the current column sort direction. Otherwise, add an initial sortby
to specify the group sort; the secondary (sorting within the groups) is subsequently
added later in the getData method.
*/
if ($sortBy === $groupBy || $this->useSecondaryGroupCondition($sortBy, $groupBy, $groupKey)) {
$this->setProperty('groupDir', $this->getProperty('dir'));
} else {
$c->sortby($groupKey, $this->getProperty('groupDir'));
}
}

/**
* Allows child classes to specify the condition(s) that will trigger the use of an
* alternate sorting routine for use in the grouping grid the child class generates
* data for - defined in setGroupSort().
*
* @param string $sortBy The data index of the selected sorting column
* @param string $groupBy The data index of the selected grouping column
* @param string $groupKey The grouping column's fully qualified SQL
* @return bool Whether the specified condition/set of conditions passes
*/
public function useSecondaryGroupCondition(string $sortBy, string $groupBy, string $groupKey): bool
{
return false;
}

/**
* Can be used to adjust the query prior to the COUNT statement
*
Expand Down
42 changes: 37 additions & 5 deletions core/src/Revolution/Processors/Security/Access/UserGroup/AccessNamespace/GetList.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,13 @@ public function initialize()
if (!empty($userGroup)) {
$this->userGroup = $this->modx->getObject(modUserGroup::class, $userGroup);
}
/*
Need to sort on the int field (authority) instead of the composite string field
(role_display) to order properly with the format of '[authority] - [role_name]'
*/
if ($this->getProperty('sort') == 'role_display') {
$this->setProperty('sort', 'authority');
}
return $initialized;
}

Expand Down Expand Up @@ -94,15 +101,40 @@ public function prepareQueryAfterCount(xPDOQuery $c)
$c->leftJoin(modAccessPolicy::class, 'Policy');
$c->select($this->modx->getSelectColumns(modAccessNamespace::class, 'modAccessNamespace'));
$c->select([
'name' => 'Target.name',
'role_name' => 'Role.name',
'policy_name' => 'Policy.name',
'policy_data' => 'Policy.data',
'name' => '`Target`.`name`',
'policy_name' => '`Policy`.`name`',
'policy_data' => '`Policy`.`data`',
'role_display' => 'CONCAT_WS(\' - \',`modAccessNamespace`.`authority`,`Role`.`name`)'
]);

if ($this->getProperty('isGroupingGrid')) {
$groupBy = $this->getProperty('groupBy');
$sortBy = $this->getProperty('sort');
if (!empty($groupBy)) {
switch ($groupBy) {
case 'name':
$groupKey = '`Target`.`name`';
break;
case 'role_display':
$groupKey = '`modAccessNamespace`.`authority`';
break;
case 'policy_name':
$groupKey = '`Policy`.`name`';
break;
default:
$groupKey = '`modAccessNamespace`.`' . $groupBy . '`';
break;
}
$this->setGroupSort($c, $sortBy, $groupBy, $groupKey);
}
}
return $c;
}

public function useSecondaryGroupCondition(string $sortBy, string $groupBy, string $groupKey): bool
{
return $sortBy === 'authority' && $groupBy === 'role_display';
}

/**
* @param xPDOObject $object
* @return array
Expand Down
44 changes: 37 additions & 7 deletions core/src/Revolution/Processors/Security/Access/UserGroup/Category/GetList.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,13 @@ public function initialize()
if (!empty($userGroup)) {
$this->userGroup = $this->modx->getObject(modUserGroup::class, $userGroup);
}

/*
Need to sort on the int field (authority) instead of the composite string field
(role_display) to order properly with the format of '[authority] - [role_name]'
*/
if ($this->getProperty('sort') == 'role_display') {
$this->setProperty('sort', 'authority');
}
return $initialized;
}

Expand Down Expand Up @@ -96,15 +102,40 @@ public function prepareQueryAfterCount(xPDOQuery $c)
$c->leftJoin(modAccessPolicy::class, 'Policy');
$c->select($this->modx->getSelectColumns(modAccessCategory::class, 'modAccessCategory'));
$c->select([
'name' => 'Target.category',
'role_name' => 'Role.name',
'policy_name' => 'Policy.name',
'policy_data' => 'Policy.data',
'name' => '`Target`.`category`',
'policy_name' => '`Policy`.`name`',
'policy_data' => '`Policy`.`data`',
'role_display' => 'CONCAT_WS(\' - \',`modAccessCategory`.`authority`,`Role`.`name`)'
]);

if ($this->getProperty('isGroupingGrid')) {
$groupBy = $this->getProperty('groupBy');
$sortBy = $this->getProperty('sort');
if (!empty($groupBy)) {
switch ($groupBy) {
case 'name':
$groupKey = '`Target`.`category`';
break;
case 'role_display':
$groupKey = '`modAccessCategory`.`authority`';
break;
case 'policy_name':
$groupKey = '`Policy`.`name`';
break;
default:
$groupKey = '`modAccessCategory`.`' . $groupBy . '`';
break;
}
$this->setGroupSort($c, $sortBy, $groupBy, $groupKey);
}
}
return $c;
}

public function useSecondaryGroupCondition(string $sortBy, string $groupBy, string $groupKey): bool
{
return $sortBy === 'authority' && $groupBy === 'role_display';
}

/**
* @param xPDOObject|modAccessResourceGroup $object
* @return array
Expand All @@ -116,7 +147,6 @@ public function prepareRow(xPDOObject $object)
if (empty($objectArray['name'])) {
$objectArray['name'] = '(' . $this->modx->lexicon('none') . ')';
}
$objectArray['authority_name'] = !empty($objectArray['role_name']) ? $objectArray['role_name'] . ' - ' . $objectArray['authority'] : $objectArray['authority'];

/* get permissions list */
$data = $objectArray['policy_data'];
Expand Down
40 changes: 33 additions & 7 deletions core/src/Revolution/Processors/Security/Access/UserGroup/Context/GetList.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,13 @@ public function initialize()
if (!empty($userGroup)) {
$this->userGroup = $this->modx->getObject(modUserGroup::class, $userGroup);
}
/*
Need to sort on the int field (authority) instead of the composite string field
(role_display) to order properly with the format of '[authority] - [role_name]'
*/
if ($this->getProperty('sort') == 'role_display') {
$this->setProperty('sort', 'authority');
}
return $initialized;
}

Expand Down Expand Up @@ -91,13 +98,36 @@ public function prepareQueryAfterCount(xPDOQuery $c)
$c->leftJoin(modAccessPolicy::class, 'Policy');
$c->select($this->modx->getSelectColumns(modAccessContext::class, 'modAccessContext'));
$c->select([
'role_name' => 'Role.name',
'policy_name' => 'Policy.name',
'policy_data' => 'Policy.data',
'policy_name' => '`Policy`.`name`',
'policy_data' => '`Policy`.`data`',
'role_display' => 'CONCAT_WS(\' - \',`modAccessContext`.`authority`,`Role`.`name`)'
]);
if ($this->getProperty('isGroupingGrid')) {
$groupBy = $this->getProperty('groupBy');
$sortBy = $this->getProperty('sort');
if (!empty($groupBy)) {
switch ($groupBy) {
case 'role_display':
$groupKey = '`modAccessContext`.`authority`';
break;
case 'policy_name':
$groupKey = '`Policy`.`name`';
break;
default:
$groupKey = '`modAccessContext`.`' . $groupBy . '`';
break;
}
$this->setGroupSort($c, $sortBy, $groupBy, $groupKey);
}
}
return $c;
}

public function useSecondaryGroupCondition(string $sortBy, string $groupBy, string $groupKey): bool
{
return $sortBy === 'authority' && $groupBy === 'role_display';
}

/**
* @param xPDOObject|modAccessContext $object
* @return array
Expand All @@ -109,10 +139,6 @@ public function prepareRow(xPDOObject $object)
if (empty($objectArray['name'])) {
$objectArray['name'] = '(' . $this->modx->lexicon('none') . ')';
}
$objectArray['authority_name'] = !empty($objectArray['role_name'])
? $objectArray['role_name'] . ' - ' . $objectArray['authority']
: $objectArray['authority']
;

/* get permissions list */
$data = $objectArray['policy_data'];
Expand Down
Loading

0 comments on commit 23d8965

Please sign in to comment.