-
Notifications
You must be signed in to change notification settings - Fork 537
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Fix] Register DataTable Columns (Employee Management Page) #8472
Conversation
Caution Review failedThe pull request is closed. WalkthroughThe pull request encompasses significant modifications to the routing configuration, employee management interface, and related components in the Angular application. Key changes include the simplification of routing logic, updates to permission checks, and enhancements in layout rendering. The Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (9)
packages/ui-core/core/src/lib/common/component-registry.types.ts (1)
75-78
: LGTM! Minor suggestion for comment consistency.The addition of 'employee-manage' to the
PageDataTableRegistryId
type is appropriate and aligns with the PR objectives. This change enhances the flexibility of the data table registry system, allowing for a dedicated employee management page.For consistency with other comments in the file, consider using sentence case for the descriptions:
* Possible values: -* - 'employee-manage': Employee management page -* - 'job-employee': Sub-page under the jobs section (Job employee management page) +* - 'employee-manage': Employee management page. +* - 'job-employee': Sub-page under the jobs section (job employee management page). */apps/gauzy/src/app/pages/employees/employees-routing.module.ts (3)
27-39
: LGTM with a minor suggestion.The changes to the main route for EmployeesComponent look good:
- The addition of
dataTableId
aligns with the updates to PageDataTableRegistryService.- The
redirectTo
property in permissions ensures consistent behavior for unauthorized access.- The
team
selector being set to false clarifies that team-related functionality is not applicable.Consider adding a brief comment explaining the purpose of the
dataTableId
for better code documentation:data: { // Identifier for the employee management data table dataTableId: 'employee-manage', // ... other properties }
67-71
: LGTM with a suggestion for consistency.The addition of selectors (
team
,project
,employee
,date
, andorganization
) set to false in the 'account' child route provides clear configuration and prevents unintended behavior.For consistency with the main route, consider ordering the selectors similarly:
selectors: { team: false, project: false, employee: false, date: false, organization: false }This ordering should be applied to all similar selector objects in the file for better readability and maintenance.
107-115
: LGTM with a suggestion for permissions consistency.The changes in the 'projects' child route maintain consistency with other child routes for selectors and align with the main route for permissions handling.
For better consistency in permissions handling across routes, consider moving the
redirectTo
property to the same level asonly
:permissions: { only: [PermissionsEnum.ALL_ORG_VIEW, PermissionsEnum.ORG_PROJECT_VIEW], redirectTo: '/pages/dashboard' }This structure should be applied consistently across all routes with permissions.
packages/ui-core/core/src/lib/utils/smart-table/server.data-source.ts (3)
107-117
: Excellent addition of comprehensive documentation!The new JSDoc comments greatly enhance the clarity and maintainability of the
addSortRequestParams
method. The description is thorough and provides valuable insights into the method's functionality, error handling, and return type.Consider adding an
@example
section to illustrate a typical usage scenario, which could further improve developers' understanding of the method.
118-142
: Improved implementation with better type safety and error handlingThe changes to the
addSortRequestParams
method are well-thought-out and improve the overall quality of the code:
- The explicit return type enhances type safety.
- The more specific type for the
orders
object improves code clarity.- The warning log for undefined directions aids in debugging.
- Explicit handling of cases with no sorting configuration makes the method more robust.
Consider a minor performance optimization: you could initialize the
orders
object only ifthis.sortConf
is truthy, avoiding unnecessary object creation. Here's a suggested refactor:protected addSortRequestParams(): { [key: string]: any } { if (this.sortConf && this.sortConf.length > 0) { const orders: { [key: string]: string } = {}; this.sortConf.forEach((fieldConf) => { if (fieldConf.direction) { orders[fieldConf.field] = fieldConf.direction.toUpperCase(); } else { console.warn(`Direction is not defined for field: ${fieldConf.field}`); } }); return { [this.conf.sortDirKey]: orders }; } return {}; }This approach eliminates the need for the final
else
block and reduces nesting.
Line range hint
1-195
: Consider applying similar improvements to other methods for consistencyThe enhancements made to
addSortRequestParams
are excellent. To maintain consistency across the class, consider applying similar improvements toaddFilterRequestParams
andaddPagerRequestParams
:
- Add comprehensive JSDoc comments to these methods.
- Improve type safety by specifying return types.
- Enhance error handling and logging where appropriate.
Here's an example of how you might update
addFilterRequestParams
:/** * Adds filter parameters to the request based on the filter configuration. * * This function processes the `filterConf` configuration and extracts the field * and its search value to create a filter object. If a field does not have a * search value, it will be skipped. The resulting filter parameters are returned * as part of an object that can be used in a request. * * @returns {Object} An object containing the filter parameters. */ protected addFilterRequestParams(): { [key: string]: any } { if (this.filterConf && this.filterConf.length > 0) { const filters: { [key: string]: any } = {}; this.filterConf.forEach(({ field, search }: IFilterConfig) => { if (search) { filters[field] = search; } }); return { [this.conf.filterFieldKey]: filters }; } return {}; }Apply similar improvements to
addPagerRequestParams
as well. This will ensure a consistent style and level of robustness across all methods in the class.apps/gauzy/src/app/pages/employees/employees.component.ts (2)
152-166
: Consider Adding Error Handling in_subscribeToQueryParams()
While subscribing to
queryParamMap
, it's good practice to handle potential errors. Consider adding anerror
handler to the observable chain to catch and manage any unexpected issues.
848-850
: Update Method Documentation forgetColumns()
The method
getColumns()
returns anIColumns
object, but the documentation mentionsany
. Update the documentation to accurately reflect the return type.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- apps/gauzy/src/app/pages/employees/employees-routing.module.ts (11 hunks)
- apps/gauzy/src/app/pages/employees/employees.component.html (3 hunks)
- apps/gauzy/src/app/pages/employees/employees.component.ts (10 hunks)
- packages/plugins/job-employee-ui/src/lib/components/job-employee/job-employee.component.ts (0 hunks)
- packages/ui-core/core/src/lib/common/component-registry.types.ts (1 hunks)
- packages/ui-core/core/src/lib/services/page/page-data-table-registry.service.ts (5 hunks)
- packages/ui-core/core/src/lib/utils/smart-table/server.data-source.ts (1 hunks)
💤 Files with no reviewable changes (1)
- packages/plugins/job-employee-ui/src/lib/components/job-employee/job-employee.component.ts
🧰 Additional context used
🔇 Additional comments (19)
apps/gauzy/src/app/pages/employees/employees-routing.module.ts (4)
50-50
: LGTM: Consistent permission handling.The addition of the
redirectTo
property in the permissions object for the 'edit/:id' route ensures consistent behavior for unauthorized access across different routes. This change aligns well with the similar modification in the main route.
80-84
: LGTM: Consistent selector configuration.The changes in the 'networks' child route mirror those in the 'account' route, maintaining consistency across child routes. This is a good practice for clarity and maintainability.
Please refer to the previous comment regarding the ordering of selectors for consistency.
93-97
: LGTM: Maintaining selector consistency.The changes in the 'rates' child route are consistent with those in the 'account' and 'networks' routes, which is excellent for maintaining a uniform configuration across child routes.
Please refer to the previous comments regarding the ordering of selectors for consistency across all child routes.
Line range hint
124-180
: LGTM: Comprehensive and consistent route configuration.The changes in the remaining child routes ('contacts', 'location', 'hiring', 'employment', and 'settings') complete the uniform configuration of selectors across all child routes of 'edit/:id'. This comprehensive update ensures consistency and clarity throughout the routing module.
These changes collectively improve the routing configuration by:
- Explicitly defining which selectors are not applicable for each route.
- Ensuring consistent permission handling across routes.
- Aligning with the updates in related services and components mentioned in the AI summary.
Remember to apply the previously suggested improvements for selector ordering and permissions structure consistently across all routes.
packages/ui-core/core/src/lib/utils/smart-table/server.data-source.ts (1)
Line range hint
1-195
: Overall, excellent improvements with room for further enhancementsThe changes made to the
addSortRequestParams
method in theServerDataSource
class are commendable. They significantly improve the method's functionality, type safety, and maintainability. The added documentation greatly enhances the understanding of the method's purpose and behavior.Key improvements:
- Comprehensive JSDoc comments
- Enhanced type safety with explicit return types
- Improved error handling and logging
- More robust handling of edge cases
To further enhance the overall quality of the class, consider:
- Applying similar improvements to
addFilterRequestParams
andaddPagerRequestParams
- Ensuring consistent error handling and logging across all methods
- Adding examples in the documentation where appropriate
These changes contribute to a more maintainable and developer-friendly codebase. Great work on the improvements made so far!
apps/gauzy/src/app/pages/employees/employees.component.html (1)
47-84
: Layout rendering logic enhanced effectivelyThe introduction of the
ngSwitch
directive to handle different layout styles improves the flexibility and maintainability of the component. The rendering logic for both the Table View and Card Grid View appears to be correctly implemented. This dynamic approach enhances the user interface by allowing easy switching between different employee views.packages/ui-core/core/src/lib/services/page/page-data-table-registry.service.ts (6)
2-2
: Import statement is correctThe import of
IColumn
andIColumns
from'angular2-smart-table'
is appropriate.
95-100
: Verify the necessity of makinggetColumnsByDataTableId
publicChanging the method
getColumnsByDataTableId
from private to public exposes it to external usage. Please confirm if this change is necessary and does not lead to unintended access to internal data structures.
108-130
: Mapping functionmapConfigToColumn
is well-implementedThe new private method
mapConfigToColumn
efficiently mapsPageDataTableRegistryConfig
objects toIColumn
objects, improving code maintainability and readability.
149-149
: Update to usegetColumnsByDataTableId
The method now utilizes
getColumnsByDataTableId
, enhancing code reuse and consistency across the service.
160-160
: Confirm logic for filtering hidden columnsThe addition of
config.hide
in the uniqueness check filters out hidden columns. Please ensure this aligns with the intended behavior for displaying columns in the data table.
171-172
: Proper use ofmapConfigToColumn
in accumulatorThe utilization of
mapConfigToColumn
within the reducer function enhances modularity and avoids code duplication.apps/gauzy/src/app/pages/employees/employees.component.ts (7)
60-60
: Improved Type Safety forsettingsSmartTable
Changing the type of
settingsSmartTable
fromobject
toSettings
enhances type safety and code clarity. This ensures that only valid settings are assigned, reducing potential runtime errors.
99-100
: Proper Injection of DependenciesInjecting
DateFormatPipe
andPageDataTableRegistryService
ensures that date formatting and data table column registration are handled efficiently within the component.
107-109
: Verify Initialization Order inngOnInit()
Ensure that calling
_registerDataTableColumns()
before_loadSmartTableSettings()
is intentional and that all required columns are registered before loading the Smart Table settings. This order affects how settings are applied to the table.
148-150
: Appropriate Use ofngAfterViewInit()
Calling
_applyTranslationOnSmartTable()
insidengAfterViewInit()
ensures that all view components are initialized before applying translations, preventing potential errors.
863-874
: Ensure Pagination Settings Are Correctly AppliedIn
_loadSmartTableSettings()
, verify thatitemsPerPage
is correctly assigned and that the pagination settings align with the application's requirements.
891-934
: Synchronize Additional Columns with Smart Table SettingsAfter registering additional columns in
_additionalColumns()
, ensure thatsettingsSmartTable
is updated accordingly so that the new columns are reflected in the Smart Table.
991-991
: Efficient Clearing of Selected ItemsThe
clearItem()
method correctly resets the selected employee state, ensuring that the UI reflects the deselection appropriately.
packages/ui-core/core/src/lib/services/page/page-data-table-registry.service.ts
Outdated
Show resolved
Hide resolved
packages/ui-core/core/src/lib/services/page/page-data-table-registry.service.ts
Show resolved
Hide resolved
☁️ Nx Cloud ReportCI is running/has finished running commands for commit a048407. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution
✅ Successfully ran 1 targetSent with 💌 from NxCloud. |
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
Release Notes
New Features
Bug Fixes
Documentation
Refactor
Chores