Skip to content

Conversation

@rishabhtc
Copy link
Collaborator

No description provided.

Copy link
Contributor

@jmgasper jmgasper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll have to test this after the migration because I'm guessing we may not import the legacy data correctly. Shouldn't matter for past tasks, but something to think about.

const existingRestrictedRole = _.find(userResources, r => restrictedRoleIds.includes(r.roleId))
if (existingRestrictedRole) {
// Get the role name for better error message
const existingRole = await getResourceRole(existingRestrictedRole.roleId, false)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[💡 performance]
The getResourceRole function is called to retrieve the role name for error messaging. If the role name is not critical for the error message, consider simplifying the error message to avoid this additional database call, which could improve performance.

handle = memberInfoFromDb.handle
const userResources = allResources.filter((r) => _.toLower(r.memberHandle) === _.toLower(handle))

let restrictedRoleIds

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ maintainability]
The variable restrictedRoleIds is initialized twice when isCreated is true, which is unnecessary and could lead to confusion. Consider initializing it once before the conditional blocks to improve clarity and maintainability.

@rishabhtc rishabhtc requested a review from jmgasper October 29, 2025 17:31
@rishabhtc rishabhtc merged commit 3bdb524 into develop Oct 30, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants