Migration final deploy #7
Conversation
Replaced handle with memberId for challenge createdBy comparison prevent submitter and restricted roles from being combined
Fixed lint error
Consider a copilot to have "full access" to a challenge (PM-2654)
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| * @returns {Promise<Array<String>>} Array of restricted role IDs | ||
| */ | ||
| async function getRestrictedRoleIds () { | ||
| if (restrictedRoleIdsCache) { |
There was a problem hiding this comment.
[maintainability]
The caching mechanism for restrictedRoleIdsCache does not account for changes in the database. If roles are updated externally, the cache might become stale. Consider implementing a cache invalidation strategy or a mechanism to refresh the cache periodically.
| */ | ||
| async function checkAccess (currentUserResources) { | ||
| const copilotRoleIds = await getCopilotResourceRoleIds() | ||
| const hasCopilotRole = _.some(currentUserResources, r => copilotRoleIds.includes(r.roleId)) |
There was a problem hiding this comment.
[💡 performance]
The use of _.some to check for copilot roles could be replaced with Array.prototype.some for better readability and performance, as native methods are generally faster.
| ) | ||
| } | ||
| // Check if user already has a restricted role (Manager, Copilot, Reviewer, etc.) | ||
| const existingRestrictedRole = _.find(userResources, r => restrictedRoleIds.includes(r.roleId)) |
There was a problem hiding this comment.
[maintainability]
The error message construction using RESTRICTED_ROLE_NAMES assumes that the list will not change. If roles are added or removed, the message might become inaccurate. Consider dynamically constructing the message based on the actual roles retrieved from the database.
| // Check if user is trying to assign a restricted role and already has submitter role | ||
| if (isCreated) { | ||
| if (restrictedRoleIds.includes(resource.roleId)) { | ||
| const existingSubmitterRole = _.find(userResources, r => r.roleId === config.SUBMITTER_RESOURCE_ROLE_ID) |
There was a problem hiding this comment.
[maintainability]
The logic for checking if a user is trying to assign a restricted role while already having a submitter role is duplicated in multiple places. Consider refactoring this logic into a separate function to improve maintainability and reduce code duplication.
| }) | ||
|
|
||
| it('copilot can manage resources without full access flags', async () => { | ||
| const originalRole = await helper.getById('ResourceRole', copilotRoleId) |
There was a problem hiding this comment.
[correctness]
Consider adding error handling for the helper.getById call. If this call fails or returns an unexpected result, it could cause the test to behave unpredictably.
| should.equal(createdResource.memberHandle.toLowerCase(), entity.memberHandle.toLowerCase()) | ||
| await assertResource(createdResource.id, createdResource) | ||
| } finally { | ||
| if (createdResource && createdResource.id) { |
There was a problem hiding this comment.
[maintainability]
The cleanup logic in the finally block is crucial for maintaining test isolation. Ensure that any errors during the prisma.resource.deleteMany call are logged or handled to avoid silent failures.
No description provided.