pkp/pkp-lib#11791 Improve performance of user list by batching permission checks #11794
Conversation
Hafsa-Naeem
force-pushed
the
i11791-stable-3_5_0-fix
branch
2 times, most recently
from
6522192 to
32a431b
Compare
classes/user/maps/Schema.php
Outdated
| // compute the page wide coverage map | ||
| private function loadUserContextPermissions(Enumerable $collection): void | ||
| { | ||
| $request = Application::get()->getRequest(); |
There was a problem hiding this comment.
I think the $auxiliaryData pattern already exists in this class to provide information like this, which would avoid us having to create another entanglement with Application::getRequest from the storage layer, which is a pattern I'd like to avoid. Better for calling code to send in the current user via $auxiliaryData. Then we can also avoid the Schema::$currentUserId property; it's a little unclear when that can be expected to contain a value.
classes/user/maps/Schema.php
Outdated
| // ensure cache has entries for ad hoc lookups | ||
| private function loadUserContextMap(array $targetIds): void | ||
| { | ||
| $targetIds = array_values(array_unique(array_map('intval', $targetIds))); |
There was a problem hiding this comment.
(It's better to use first-class callable syntax: intval(...) rather than 'intval')
classes/user/maps/Schema.php
Outdated
| THEN 0 ELSE 1 END AS manages_all', | ||
| [ (int) Role::ROLE_ID_MANAGER, (int) $managerId ] | ||
| ) | ||
| ->whereIn('u.user_id', array_map('intval', $targetIds)) |
There was a problem hiding this comment.
As above, use first-class callable syntax.
classes/user/maps/Schema.php
Outdated
| /** | ||
| * extend $this->managesAllMap for the given manager and target ids | ||
| */ | ||
| private function buildUserContextManagementMap(int $managerId, array $targetIds): void |
There was a problem hiding this comment.
$targetIds is not clear; call it e.g. $managedUserIds instead.
classes/user/maps/Schema.php
Outdated
| ->get(); | ||
|
|
||
| foreach ($rows as $r) { | ||
| $this->managesAllMap[(int) $r->user_id] = ((int) $r->manages_all) === 1; |
There was a problem hiding this comment.
Don't use an exact === test; some DBMS configurations return numeric types as strings.
classes/user/maps/Schema.php
Outdated
| public string $schema = PKPSchemaService::SCHEMA_USER; | ||
|
|
||
| // page scoped cache to check if current user manage all contexts of target user | ||
| private array $managesAllMap = []; |
There was a problem hiding this comment.
Rather than using a class property that can be set in some (but not all) cases, and which might get left around and accidentally affect re-use of the Schema object, use a local variable in cases where this is useful. (In other circumstances we use properties, but I think those are appropriate because the values are not as arbitrary as they are here -- where they are only relevant to a single user ID.)
Hafsa-Naeem
force-pushed
the
i11791-stable-3_5_0-fix
branch
2 times, most recently
from
56130bb to
9c34b62
Compare
Hafsa-Naeem
force-pushed
the
i11791-stable-3_5_0-fix
branch
from
9c34b62 to
10ea169
Compare
2 participants
for #11791