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', |
There was a problem hiding this comment.
Sure however i personally do prefer not trying to force such a prefix over readability. Imo it's a limit for "admin users", for which ADMIN_USERS_LIMIT make sense to me, but USERS_ADMIN_LIMIT makes me think twice about whether the "admin" applies to the limit or the user 🤔 In the end thats a tiny detail tho, either way would be an improvement over the longer form i think 😄
| 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? 🤔
There was a problem hiding this comment.
Im wondering if it would be necessary on large database with tons of users/roles 🤔 Not sure atm
Co-authored-by: Brainslug <br41nslug@users.noreply.github.com>
Co-authored-by: Brainslug <br41nslug@users.noreply.github.com>
Co-authored-by: Brainslug <br41nslug@users.noreply.github.com>
Scope
What's changed:
Potential Risks / Drawbacks
Review Notes / Questions
Implements the limits mentioned in issue #21981 for Public Registration #22125