[Enhancement] Introduce SELECT_EMPLOYEE Permission & Allow Employee to Assign Managers/Members in Project Creation #8836
Conversation
WalkthroughThis pull request introduces a new permission, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Controller
participant Service
participant DB
User->>Controller: GET /working or /working/count
Controller->>Service: Invoke getFilterQuery()/getWorkingEmployees()
alt User has both CHANGE_SELECTED_EMPLOYEE and SELECT_EMPLOYEE
Service->>DB: Retrieve complete employee data
else Permissions missing
Service->>DB: Retrieve filtered employee data by user ID
end
DB-->>Service: Return employee data
Service-->>Controller: Return processed data
Controller-->>User: Send response with employee data
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (12)
🚧 Files skipped from review as they are similar to previous changes (10)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🔇 Additional comments (2)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
View your CI Pipeline Execution ↗ for commit 3492a38.
☁️ Nx Cloud last updated this comment at |
| @@ -70,7 +70,7 @@ export class EmployeeController extends CrudController<Employee> { | |||
| status: HttpStatus.NOT_FOUND, | |||
| description: 'No working employees found' | |||
| }) | |||
| @Permissions(PermissionsEnum.CHANGE_SELECTED_EMPLOYEE) | |||
| @Permissions(PermissionsEnum.SELECT_EMPLOYEE) | |||
There was a problem hiding this comment.
@samuelmbabhazi So make sure we use both permisisons here.
There was a problem hiding this comment.
@rahul-rocket
We could say that CHANGE_SELECTED_EMPLOYEE includes SELECT_EMPLOYEE, meaning it's not possible to have the first without also having the second. Since our permission guard does not support an "OR" condition, using only SELECT_EMPLOYEE ensures that both cases are handled correctly without requiring changes to the guard logic.
There was a problem hiding this comment.
@samuelmbabhazi What if someone have CHANGE_SELECTED_EMPLOYEE but not SELECT_EMPLOYEE??
There was a problem hiding this comment.
@rahul-rocket SELECT_EMPLOYEE is really a low level permission compared to CHANGE_SELECTED_EMPLOYEE if we can say so,
so I can't find a case where a user could have CHANGE_SELECTED_EMPLOYEE without having SELECT_EMPLOYEE
There was a problem hiding this comment.
@samuelmbabhazi I think you don't understand what i mean. We have to add both permissions in there.
@permissions(PermissionsEnum.CHANGE_SELECTED_EMPLOYEE, PermissionsEnum.SELECT_EMPLOYEE)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/ui-core/i18n/assets/i18n/es.json (1)
2047-2047: Looks good: Spanish translation for SELECT_EMPLOYEE permission added correctly.The Spanish translation "Seleccionar empleado" for the new SELECT_EMPLOYEE permission is correct and aligns with the PR objective to introduce this permission.
There's a minor consistency issue: at line 392 this key has a period at the end (
"SELECT_EMPLOYEE": "Seleccione empleado.") but this new entry doesn't have one. Consider standardizing punctuation across all occurrences of this key in the file.packages/ui-core/i18n/assets/i18n/en.json (1)
2154-2155: New Permission Translation Keys AddedThe new keys
"SELECT_EMPLOYEE": "Select Employee"and"CHANGE_SELECTED_EMPLOYEE": "Change Selected Employee"have been introduced. Their wording is clear and consistent with similar UI action labels in the file. Please ensure that these additions are propagated to other language files as needed for full localization support.packages/core/src/lib/employee/employee.service.ts (1)
410-418: Ensure consistent documentation for the newly added permission check.The comment on line 410 only mentions the
CHANGE_SELECTED_EMPLOYEEpermission but not the newly addedSELECT_EMPLOYEEpermission. This should be updated to reflect both permissions being checked.- // Check for permission CHANGE_SELECTED_EMPLOYEE + // Check for permissions CHANGE_SELECTED_EMPLOYEE and SELECT_EMPLOYEE if ( !RequestContext.hasPermission(PermissionsEnum.CHANGE_SELECTED_EMPLOYEE) && !RequestContext.hasPermission(PermissionsEnum.SELECT_EMPLOYEE) ) {packages/core/src/lib/employee/employee.controller.ts (1)
292-303: Consider updating findById method to check for SELECT_EMPLOYEE permissionFor consistency with your changes to other methods, consider updating the permission check in the
findByIdmethod to also check for theSELECT_EMPLOYEEpermission alongsideCHANGE_SELECTED_EMPLOYEE.-...(RequestContext.hasPermission(PermissionsEnum.CHANGE_SELECTED_EMPLOYEE) +...(RequestContext.hasPermission(PermissionsEnum.CHANGE_SELECTED_EMPLOYEE) || + RequestContext.hasPermission(PermissionsEnum.SELECT_EMPLOYEE) ? { id } : { id: RequestContext.currentEmployeeId() })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
packages/contracts/src/lib/role-permission.model.ts(2 hunks)packages/core/src/lib/employee/employee.controller.ts(2 hunks)packages/core/src/lib/employee/employee.service.ts(1 hunks)packages/core/src/lib/role-permission/default-role-permissions.ts(4 hunks)packages/ui-core/i18n/assets/i18n/ach.json(1 hunks)packages/ui-core/i18n/assets/i18n/ar.json(1 hunks)packages/ui-core/i18n/assets/i18n/bg.json(1 hunks)packages/ui-core/i18n/assets/i18n/de.json(1 hunks)packages/ui-core/i18n/assets/i18n/en.json(1 hunks)packages/ui-core/i18n/assets/i18n/es.json(1 hunks)packages/ui-core/i18n/assets/i18n/fr.json(1 hunks)packages/ui-core/i18n/assets/i18n/he.json(1 hunks)packages/ui-core/i18n/assets/i18n/it.json(1 hunks)packages/ui-core/i18n/assets/i18n/nl.json(1 hunks)packages/ui-core/i18n/assets/i18n/pl.json(1 hunks)packages/ui-core/i18n/assets/i18n/pt.json(1 hunks)packages/ui-core/i18n/assets/i18n/ru.json(1 hunks)packages/ui-core/i18n/assets/i18n/zh.json(1 hunks)packages/ui-core/shared/src/lib/employee/employee-multi-select/employee-multi-select.component.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: test
- GitHub Check: build
🔇 Additional comments (22)
packages/ui-core/i18n/assets/i18n/ar.json (1)
2039-2039: Translation added correctly for new permissionThe Arabic translation for the new
SELECT_EMPLOYEEpermission has been added appropriately. This aligns with the PR's objective of introducing this new permission to the system.packages/ui-core/i18n/assets/i18n/ru.json (1)
2082-2082: Well-implemented localization addition for the new permission.The Russian translation for "SELECT_EMPLOYEE" has been properly added as "Выбрать сотрудника" (meaning "Select employee"). This translation correctly matches the naming pattern of other similar permissions in this section and provides the necessary localization support for the new permission being introduced in this PR.
packages/ui-core/i18n/assets/i18n/ach.json (1)
1872-1872: Appropriate translation added for new permission.The translation "Yer Employee" for the new
SELECT_EMPLOYEEpermission key is correctly added to the localization file. This aligns with the PR objective of introducing theSELECT_EMPLOYEEpermission throughout the system.packages/ui-core/i18n/assets/i18n/he.json (1)
2075-2075: Properly adds Hebrew translation for new SELECT_EMPLOYEE permissionThe addition of the Hebrew translation "בחר עובד" for the new SELECT_EMPLOYEE permission is correct and follows the established format in the permissions section. This translation properly supports the new permission being introduced in this PR, which will allow employees to select other employees when assigning managers/members in project creation.
packages/ui-core/i18n/assets/i18n/fr.json (1)
2042-2042: Translation is correctly added for the new permission.The new
SELECT_EMPLOYEEpermission key has been properly translated to "Sélectionner un employé" in French, which is grammatically correct and matches the intent of the feature described in the PR.packages/ui-core/i18n/assets/i18n/bg.json (1)
2109-2109: Translation added correctly for new permissionThe Bulgarian translation "Избор на служител" (which means "Select Employee") has been added for the new
SELECT_EMPLOYEEpermission. This aligns well with the PR objective of introducing this new permission to the system.packages/ui-core/i18n/assets/i18n/it.json (1)
2045-2045: Appropriate translation for the new 'SELECT_EMPLOYEE' permission.The added translation "Seleziona dipendente" correctly represents "Select employee" in Italian, maintaining consistency with the existing UI terminology. This addition properly supports the new permission introduced in this pull request.
packages/ui-core/i18n/assets/i18n/pt.json (1)
2045-2045: Proper translation added for SELECT_EMPLOYEE permission.The new translation entry "SELECT_EMPLOYEE": "Selecionar funcionário" has been correctly added to the Portuguese localization file, allowing the system to display this permission name in Portuguese. This entry is part of the broader permission system enhancement described in the PR.
packages/ui-core/i18n/assets/i18n/nl.json (1)
2045-2045: Translation addition looks good.The Dutch translation "Selecteer werknemer" has been correctly added for the new permission key "SELECT_EMPLOYEE", which aligns with the PR's objective of introducing this new permission to the system.
packages/ui-core/i18n/assets/i18n/zh.json (1)
2045-2045: Appropriate translation added for the new SELECT_EMPLOYEE permissionThe appropriate Chinese translation "选择员工" (meaning "Select Employee") has been added for the new permission introduced in this PR. This translation is consistent with the naming convention used for other permission translations in the file.
packages/ui-core/i18n/assets/i18n/pl.json (1)
2045-2045: Translation key added correctly for the new permission.The translation for "SELECT_EMPLOYEE" has been appropriately added to the Polish language file, providing the translation "Wybierz pracownika" for the newly introduced permission. This addition aligns with the PR objective of introducing the SELECT_EMPLOYEE permission.
packages/ui-core/shared/src/lib/employee/employee-multi-select/employee-multi-select.component.ts (1)
164-167:✅ Verification successful
Permission check logic enhanced with the new SELECT_EMPLOYEE permission.
The component now checks for both
CHANGE_SELECTED_EMPLOYEEandSELECT_EMPLOYEEpermissions, making the access control more granular. This change aligns with the PR objective of introducing theSELECT_EMPLOYEEpermission.
🏁 Script executed:
#!/bin/bash # Check where else this new permission is being used in the codebase echo "Checking usage of SELECT_EMPLOYEE permission across the codebase..." rg -A 2 "SELECT_EMPLOYEE" --glob "*.ts"Length of output: 5329
Permission Check Logic Updated and Consistent
The updated logic inpackages/ui-core/shared/src/lib/employee/employee-multi-select/employee-multi-select.component.tsnow correctly verifies bothCHANGE_SELECTED_EMPLOYEEand the newSELECT_EMPLOYEEpermissions. Our review confirms that this change is consistent with its usage across the codebase (e.g., within controllers, services, and related UI components), aligning perfectly with the PR’s intent to enhance access control.packages/core/src/lib/employee/employee.service.ts (1)
411-414:❓ Verification inconclusive
Permission check updated to include the new SELECT_EMPLOYEE permission.
The service now checks for both
CHANGE_SELECTED_EMPLOYEEandSELECT_EMPLOYEEpermissions before filtering employees to only the current employee. This is consistent with the frontend component changes and enables more flexible employee selection based on permissions.
🏁 Script executed:
#!/bin/bash # Check if the permission is properly defined in the enum echo "Checking if SELECT_EMPLOYEE is defined in PermissionsEnum..." rg -A 1 -B 1 "enum PermissionsEnum" --glob "*.ts" | rg -A 20 -B 1 "SELECT_EMPLOYEE" echo "Checking if SELECT_EMPLOYEE is added to default role permissions..." rg "SELECT_EMPLOYEE.*DEFAULT_ROLE_PERMISSIONS" --glob "*.ts"Length of output: 422
Permission Check Update – Manual Verification Required
The updated permission logic in
packages/core/src/lib/employee/employee.service.ts(lines 411–414) appears to correctly check for bothCHANGE_SELECTED_EMPLOYEEandSELECT_EMPLOYEE. However, our initial search scripts didn’t return any evidence thatSELECT_EMPLOYEEis defined in thePermissionsEnumor added to the default role permissions.Please verify that:
SELECT_EMPLOYEEis defined inPermissionsEnum.SELECT_EMPLOYEEis included in the default role permissions configuration.Once these points are confirmed manually, the permission check update will be fully validated.
packages/ui-core/i18n/assets/i18n/de.json (1)
2041-2041: Translation for new permission key looks good.The addition of "SELECT_EMPLOYEE": "Mitarbeiter auswählen" provides the correct German translation for the newly introduced permission. The translation is properly placed within the PERMISSIONS section and maintains consistency with the other permission keys in the file.
packages/core/src/lib/role-permission/default-role-permissions.ts (4)
29-29: Addition of SELECT_EMPLOYEE permission to SUPER_ADMIN role looks goodThis addition makes sense for the SUPER_ADMIN role, giving them the ability to select employees as expected for this highest permission level.
233-233: Addition of SELECT_EMPLOYEE permission to ADMIN role looks goodThis is an appropriate permission for the ADMIN role, allowing them to select employees within the organization they manage.
450-451: Addition of SELECT_EMPLOYEE permission to DATA_ENTRY role looks goodThe permission has been correctly added to the DATA_ENTRY role. This allows users with this role to select employees which would be necessary for data entry operations involving employee data.
465-465: Addition of SELECT_EMPLOYEE permission to EMPLOYEE role looks goodThis gives employees the ability to select other employees which may be needed for team-based operations or project assignments.
packages/contracts/src/lib/role-permission.model.ts (2)
110-110: Addition of SELECT_EMPLOYEE to PermissionsEnum is correctThe new permission has been properly added to the PermissionsEnum, following the established naming convention and format.
248-248:❓ Verification inconclusive
SELECT_EMPLOYEE permission correctly added to GENERAL permissions group
The permission has been appropriately added to the GENERAL permission group, making it available for assignment to different roles.
Make sure translations for this permission are added to the localization files to ensure proper UI display.
🏁 Script executed:
#!/bin/bash # Check if translations exist for SELECT_EMPLOYEE in localization files echo "Checking for SELECT_EMPLOYEE translations in localization files..." fd . --glob "packages/ui-core/i18n/assets/i18n/**" --exec grep -l "SELECT_EMPLOYEE" {} \;Length of output: 359
Follow-Up: Verify Translations for SELECT_EMPLOYEE
The
SELECT_EMPLOYEEpermission has been correctly added to the GENERAL permissions group.However, the original translation-check script failed because the specified path (
packages/ui-core/i18n/assets/i18n/**) is not recognized as a valid directory. This makes it unclear whether the required translations forSELECT_EMPLOYEEexist.Action Item: Please manually verify (or re-run with an updated search command) that the translations for
SELECT_EMPLOYEEare present in the proper localization directory (likely somewhere underpackages/ui-core/i18n). For example, you might try:# Recheck translations using a more general search in the i18n folder rg "SELECT_EMPLOYEE" packages/ui-core/i18nOnce verified, if translations are missing, add the necessary entries to ensure proper UI display.
packages/core/src/lib/employee/employee.controller.ts (2)
73-73: Updated permission requirement for findAllWorkingEmployees looks goodThe endpoint now correctly requires both
CHANGE_SELECTED_EMPLOYEEandSELECT_EMPLOYEEpermissions, which aligns with the PR objective of enhancing permission control for employee selection.
99-99: Updated permission requirement for findAllWorkingEmployeesCount looks goodThis endpoint now properly requires both
CHANGE_SELECTED_EMPLOYEEandSELECT_EMPLOYEEpermissions, ensuring consistent permission requirements with the related endpoint.
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 11719210 | Triggered | Nx Cloud Token | 5bcbef2 | nx.json | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
|
@samuelmbabhazi please fix merge conflict and best if you rebase on top of latest on develop |
PR
Please note: we will close your PR without comment if you do not check the boxes above and provide ALL requested information.
Summary by CodeRabbit
New Features
Bug Fixes
Chores