Add user limits #22479
Add user limits #22479
Conversation
|
| 'USERS_ACTIVE_LIMIT_ADMIN_ACCESS', | ||
| 'USERS_ACTIVE_LIMIT_APP_ACCESS', | ||
| 'USERS_ACTIVE_LIMIT_API_ACCESS_ACCESS', |
There was a problem hiding this comment.
The order of words here is not very intuitive to me and the use of "active" and "access" feel redundant 🤔 Could we perhaps simplify to something like this: (similar and consistent with EXTENSIONS_LIMIT):
| 'USERS_ACTIVE_LIMIT_ADMIN_ACCESS', | |
| 'USERS_ACTIVE_LIMIT_APP_ACCESS', | |
| 'USERS_ACTIVE_LIMIT_API_ACCESS_ACCESS', | |
| 'ADMIN_USERS_LIMIT', | |
| 'APP_USERS_LIMIT', | |
| 'API_USERS_LIMIT', |
There was a problem hiding this comment.
Could we prefix with USERS instead?
| 'USERS_ACTIVE_LIMIT_ADMIN_ACCESS', | |
| 'USERS_ACTIVE_LIMIT_APP_ACCESS', | |
| 'USERS_ACTIVE_LIMIT_API_ACCESS_ACCESS', | |
| 'USERS_ADMIN_LIMIT', | |
| 'USERS_APP_LIMIT', | |
| 'USERS_API_LIMIT', |
| * Get the role type counts by role IDs | ||
| */ | ||
| export async function getRoleCountsByRoles(db: Knex, roles: string[]): Promise<UserCount> { | ||
| const counts: UserCount = { |
There was a problem hiding this comment.
Should this type be aliased or renamed to be consistent with the function name? I would expect the getRoleCounts function to return a RoleCount type over a UserCount (or perhaps the types needs a more generic name like CountResult)
| const counts: UserCount = { | |
| const counts: CountResult = { |
There was a problem hiding this comment.
I was thinking about that but had left it untouched, naming is hard. CountResult might be a little generalised, how does AccessCount or AccessTypeCount sound? Or any other keyword is better suited?
| const counts: UserCount = { | |
| const counts: AccessCount = { |
| const counts: UserCount = { | |
| const counts: AccessTypeCount = { |
|
|
||
| for (const role of roles) { | ||
| if (typeof role === 'object') { | ||
| if ('admin_access' in role && role['admin_access'] === true) { |
There was a problem hiding this comment.
Since roles here are user provided, do we have to make use of the toBoolean util here?
| if ('admin_access' in role && role['admin_access'] === true) { | |
| if ('admin_access' in role && toBoolean(role['admin_access'])) { |
There was a problem hiding this comment.
Hmm, I believe our usage of toBoolean is for values fetched from DB to account for the inconsistencies. User provided values aren't cast with such magic elsewhere within our codebase. 🤔
There was a problem hiding this comment.
Why is the checkIncreasedUserLimits function in a file called get-role-counts-by-users.ts and the getRoleCountsByUsers function in a file called check-increased-user-limits.ts? 😂
| export async function checkIncreasedUserLimits(db: Knex, increasedUserCounts: UserCount): Promise<void> { | ||
| if (!increasedUserCounts.admin && !increasedUserCounts.app && !increasedUserCounts.api) return; | ||
|
|
||
| const userCounts = await getUserCount(db); |
There was a problem hiding this comment.
Im wondering if this is something we should cache for large database? 🤔 will do some testing on a larger DB
There was a problem hiding this comment.
Ah... Interesting! Are you thinking of caching the limits in Redis? 🤔
Co-authored-by: Brainslug <[email protected]>
Co-authored-by: Brainslug <[email protected]>
Co-authored-by: Brainslug <[email protected]>
Scope
What's changed:
Potential Risks / Drawbacks
Review Notes / Questions
Implements the limits mentioned in issue #21981 for Public Registration #22125