Yakymiv/officials list #2713
Yakymiv/officials list #2713
Conversation
WalkthroughThis pull request refactors the employee management functionality to focus on official employees. Changes include adding a new RNOKPP column in the user list component, updating models to include a provider ID and new official employee definitions, and renaming service methods, actions, selectors, and states to reflect the official employee domain. The utility function and component logic, along with associated tests and styles, have been updated to support these changes. Overall, the adjustments transition data handling, API calls, and UI display from regular employees to official employees. Changes
Sequence Diagram(s)sequenceDiagram
participant Component as EmployeesComponent
participant Store as ProviderState
participant Service as EmployeeService
participant API as Backend API
Component->>Store: Dispatch GetFilteredOfficialEmployees(filterParams)
Store->>Service: getFilteredOfficialEmployees(filterParams)
Service->>API: HTTP GET /api/v1/providers/{providerId}/officials/Get
API-->>Service: Return OfficialEmployee data
Service-->>Store: Emit SearchResponse<OfficialEmployee[]>
Store-->>Component: Update officialEmployees state
Suggested reviewers
Poem
Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
src/app/shell/personal-cabinet/provider/employees/employees.component.scss (1)
41-45: Update media queries to use new class name.The media queries still reference the old class name
.search-areawhile it has been renamed to.search-container. Update the selectors for consistency.@media (max-width: 1070px) { .tab-wrapper { bottom: 0; } - .search-area { + .search-container { display: flex; justify-content: flex-start; } } @media (max-width: 1200px) { - .search-area { + .search-container { position: static; margin-bottom: 1rem; justify-content: space-between; .mat-menu-panel.header_menu { background-color: #fff; } .mat-menu-item { color: rgba(0, 0, 0, 0.87); } } }Also applies to: 48-61
src/app/shell/personal-cabinet/provider/employees/employees.component.html (1)
51-51: Update empty list message to reference officials.The empty list message still references employees while the component has transitioned to handling officials. Update the message for consistency.
- <app-no-result-card [title]="noEmployees"></app-no-result-card> + <app-no-result-card [title]="noOfficials"></app-no-result-card>
🧹 Nitpick comments (9)
src/app/shell/personal-cabinet/provider/employees/employees.component.ts (4)
17-25: Imports for officials transition
The new imports align with the transition from "employees" to "officials." The usage ofEmployeeRolesuggests you're still reusing employee roles for officials. If that's intentional, no change is needed. Otherwise, consider introducing a dedicatedOfficialRoleif official roles differ meaningfully.
54-54: Provider ID initialization
InitializingproviderIdwith an empty string is fine, but you might consider making itnullorundefinedto clearly indicate that it's uninitialized until your subscription sets the actual ID.
58-58: Column naming
Including'rnokpp'is understandable for business needs. Ensure that the UI or translation layer (if any) provides user-friendly labels rather than exposing raw column identifiers.
189-201: Repeated calls togetFilteredOfficials
You're callinggetFilteredOfficialsin multiple places (page change, filter changes, etc.). While it's valid, consider consolidating these calls or implementing a single entry point for code reuse and clearer readability.src/app/shared/models/official.model.ts (1)
3-18: Consider using typed dates
activeFromandactiveToare defined as strings. If these represent dates, consider using a Date type or a well-defined date handling library to ensure type safety and reduce parsing errors throughout the application.src/app/shared/models/employee.model.ts (1)
24-40: Constructor parameter expansions
AddingproviderIdto theEmployeeconstructor is consistent with the optional property above. However, note that you're introducing many parameters. Consider accepting an object instead of individual parameters for improved readability, e.g.,constructor({ info, userId, workshopIds, providerId, accountStatus }: YourType).src/app/shared/services/employee/employee.service.ts (2)
27-36: Consider updating method documentation.The method documentation still references "provider admins" instead of "officials".
/** - * This method get provider admisn with filter parameters + * This method gets filtered officials for a provider */
16-87: Consider completing the transition to officials.The service still contains methods referencing employees (getEmployeeById, createEmployee, etc.) while transitioning to officials. Consider updating these methods to maintain consistency.
src/app/shared/utils/utils.ts (1)
144-155: Consider reusing the getFullName method.The full name concatenation could be simplified by reusing the existing
getFullNamemethod.updatedOfficials.push({ id: admin.id, - pib: `${admin.lastName} ${admin.firstName} ${admin.middleName}`, + pib: this.getFullName(admin), role: admin.position, rnokpp: admin.rnokpp });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
src/assets/i18n/en.jsonis excluded by!src/assets/**and included bysrc/**src/assets/i18n/uk.jsonis excluded by!src/assets/**and included bysrc/**
📒 Files selected for processing (12)
src/app/shared/components/users-list/users-list.component.html(1 hunks)src/app/shared/models/employee.model.ts(1 hunks)src/app/shared/models/official.model.ts(1 hunks)src/app/shared/models/users-table.ts(1 hunks)src/app/shared/services/employee/employee.service.ts(2 hunks)src/app/shared/store/provider.actions.ts(1 hunks)src/app/shared/store/provider.state.ts(7 hunks)src/app/shared/utils/utils.ts(2 hunks)src/app/shell/personal-cabinet/provider/employees/employees.component.html(4 hunks)src/app/shell/personal-cabinet/provider/employees/employees.component.scss(1 hunks)src/app/shell/personal-cabinet/provider/employees/employees.component.spec.ts(1 hunks)src/app/shell/personal-cabinet/provider/employees/employees.component.ts(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: SonarCloud
- GitHub Check: build_and_test
🔇 Additional comments (10)
src/app/shell/personal-cabinet/provider/employees/employees.component.ts (4)
38-39: Good use of NGXS selector
Selectingofficialsfrom the provider state follows the new data flow for managing officials instead of employees. Implementation looks clean.
47-49: EnsurefilteredOfficialsis updated
filteredOfficialsis initialized toofficialsDatabut never reassigned after retrieval. If you intend to filter data (e.g., via search), ensure updates toofficialsDataalso propagate tofilteredOfficials.
77-82: Pagination parameter updates
Usage ofUtil.setFromPaginationParamin bothngOnInitand before fetching officials is consistent. Ensure you handle edge cases wherethis.officialsmight not be defined yet.
204-207: Data transformation
Transforming rawOfficial[]intoofficialsDatais good. If further transformations or checks are needed, consider extracting this logic to a dedicated helper or service for maintainability.src/app/shared/models/employee.model.ts (1)
7-7: Provider ID usage
MakingproviderIdoptional withinEmployeeParametershelps contextualize employee queries by provider. This aligns well with your transition.src/app/shared/models/users-table.ts (1)
52-57: LGTM! Well-structured interface for official data.The new
OfficialTableDatainterface is properly defined with appropriate types for all properties.src/app/shared/services/employee/employee.service.ts (1)
7-7: LGTM! New Official model import added.src/app/shell/personal-cabinet/provider/employees/employees.component.spec.ts (1)
101-105: LGTM! Mock parameters updated correctly.The
mockFilterParamsobject has been properly updated to include the newproviderIdproperty.src/app/shared/utils/utils.ts (1)
139-143: LGTM! Method documentation is clear and accurate.src/app/shared/components/users-list/users-list.component.html (1)
27-33: New RNOKPP Column Addition:
The new column forrnokppis well integrated. The<ng-container>withmatColumnDef="rnokpp"follows the same structure as the other columns—using a translatable header with a sort directive and a corresponding data cell displayingelement.rnokpp. Please ensure that the underlying data model and any related services are updated to consistently support thernokppproperty for each official.
| export class GetFilteredOfficials { | ||
| static readonly type = '[provider] get filtered Employee users'; | ||
| constructor(public payload: EmployeeParameters) {} | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update static type string to match class name.
The static type string still references "Employee" while the class has been renamed to handle officials. This inconsistency should be fixed for better maintainability.
export class GetFilteredOfficials {
- static readonly type = '[provider] get filtered Employee users';
+ static readonly type = '[provider] get filtered Official users';
constructor(public payload: EmployeeParameters) {}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export class GetFilteredOfficials { | |
| static readonly type = '[provider] get filtered Employee users'; | |
| constructor(public payload: EmployeeParameters) {} | |
| } | |
| export class GetFilteredOfficials { | |
| static readonly type = '[provider] get filtered Official users'; | |
| constructor(public payload: EmployeeParameters) {} | |
| } |
| @Action(providerActions.GetFilteredOfficials) | ||
| getFilteredEmployees( | ||
| { patchState }: StateContext<ProviderStateModel>, | ||
| { payload }: providerActions.GetFilteredEmployees | ||
| ): Observable<SearchResponse<Employee[]>> { | ||
| { payload }: providerActions.GetFilteredOfficials | ||
| ): Observable<SearchResponse<Official[]>> { | ||
| patchState({ isLoading: true }); | ||
| return this.employeeService | ||
| .getFilteredEmployees(payload) | ||
| .pipe(tap((employees: SearchResponse<Employee[]>) => patchState({ employees: employees ?? EMPTY_RESULT, isLoading: false }))); | ||
| .getFilteredOfficials(payload) | ||
| .pipe(tap((officials: SearchResponse<Official[]>) => patchState({ officials: officials ?? EMPTY_RESULT, isLoading: false }))); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update method name to match action name.
The method name getFilteredEmployees should be renamed to getFilteredOfficials to match the action name and maintain consistency with the transition to officials.
@Action(providerActions.GetFilteredOfficials)
-getFilteredEmployees(
+getFilteredOfficials(
{ patchState }: StateContext<ProviderStateModel>,
{ payload }: providerActions.GetFilteredOfficials
): Observable<SearchResponse<Official[]>> {
patchState({ isLoading: true });
return this.employeeService
.getFilteredOfficials(payload)
.pipe(tap((officials: SearchResponse<Official[]>) => patchState({ officials: officials ?? EMPTY_RESULT, isLoading: false })));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Action(providerActions.GetFilteredOfficials) | |
| getFilteredEmployees( | |
| { patchState }: StateContext<ProviderStateModel>, | |
| { payload }: providerActions.GetFilteredEmployees | |
| ): Observable<SearchResponse<Employee[]>> { | |
| { payload }: providerActions.GetFilteredOfficials | |
| ): Observable<SearchResponse<Official[]>> { | |
| patchState({ isLoading: true }); | |
| return this.employeeService | |
| .getFilteredEmployees(payload) | |
| .pipe(tap((employees: SearchResponse<Employee[]>) => patchState({ employees: employees ?? EMPTY_RESULT, isLoading: false }))); | |
| .getFilteredOfficials(payload) | |
| .pipe(tap((officials: SearchResponse<Official[]>) => patchState({ officials: officials ?? EMPTY_RESULT, isLoading: false }))); | |
| } | |
| @Action(providerActions.GetFilteredOfficials) | |
| getFilteredOfficials( | |
| { patchState }: StateContext<ProviderStateModel>, | |
| { payload }: providerActions.GetFilteredOfficials | |
| ): Observable<SearchResponse<Official[]>> { | |
| patchState({ isLoading: true }); | |
| return this.employeeService | |
| .getFilteredOfficials(payload) | |
| .pipe(tap((officials: SearchResponse<Official[]>) => patchState({ officials: officials ?? EMPTY_RESULT, isLoading: false }))); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/app/shared/services/employee/employee.service.spec.ts (1)
149-158: Fix typo in method name.The method name contains a typo.
- service.reinvateEmployee(employee).subscribe((response) => { + service.reinviteEmployee(employee).subscribe((response) => {src/app/shell/personal-cabinet/provider/employees/employees.component.ts (1)
57-57: Consider adding tooltips for RNOKPP column.Since RNOKPP is an abbreviation, consider adding tooltips to improve user experience.
src/app/shared/store/provider.state.ts (1)
593-605: Consider adding error handling for block operation.The success handler should consider potential edge cases where the block operation might need to be reverted.
@Action(providerActions.OnBlockEmployeeSuccess) onBlockEmployeeSuccess( { dispatch }: StateContext<ProviderStateModel>, { payload, filterParams }: providerActions.OnBlockEmployeeSuccess ): void { + try { dispatch([ new providerActions.GetFilteredOfficials(filterParams), new ShowMessageBar({ message: payload.isBlocked ? SnackbarText.blockPerson : SnackbarText.unblockPerson, type: 'success' }) ]); + } catch (error) { + dispatch(new ShowMessageBar({ + message: SnackbarText.error, + type: 'error' + })); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/app/shared/services/employee/employee.service.spec.ts(1 hunks)src/app/shared/store/provider.actions.ts(1 hunks)src/app/shared/store/provider.state.ts(7 hunks)src/app/shell/personal-cabinet/provider/employees/employees.component.spec.ts(5 hunks)src/app/shell/personal-cabinet/provider/employees/employees.component.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/shared/store/provider.actions.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build_and_test
- GitHub Check: SonarCloud
🔇 Additional comments (10)
src/app/shared/services/employee/employee.service.spec.ts (3)
31-43: LGTM! Well-structured test for GET employee by ID.The test properly validates the request method, URL, and response handling.
45-81: LGTM! Comprehensive test for GET filtered officials.The test thoroughly validates:
- Filter parameters structure
- Request URL construction
- Response handling with the new Official model
83-95: LGTM! Good test coverage for employee creation.The test properly validates the request method, URL, body, and response handling.
src/app/shell/personal-cabinet/provider/employees/employees.component.ts (3)
37-38: LGTM! Proper state selector update.The selector has been correctly updated to use the new officials state.
46-48: LGTM! Well-structured data properties.The properties are properly typed and initialized for managing officials data.
203-206: LGTM! Proper subscription handling for officials data.The subscription correctly updates the component's data structures and uses the Util helper for table data transformation.
src/app/shell/personal-cabinet/provider/employees/employees.component.spec.ts (2)
86-94: LGTM! Good test coverage for pagination.The test properly validates page changes and method calls.
96-104: LGTM! Well-structured test for items per page.The test thoroughly validates the update of filter parameters and page reset.
src/app/shared/store/provider.state.ts (2)
41-41: LGTM! Proper state model update.The state model correctly includes the officials property with proper typing.
326-335: LGTM! Well-structured action handler for filtered officials.The action handler properly:
- Updates loading state
- Calls the service method
- Handles the response with proper error handling
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/app/shared/store/provider.state.ts (1)
326-339: 🛠️ Refactor suggestionUpdate method name to match action name.
The method name should be renamed to match the action name and maintain consistency with the transition to officials.
Apply this diff to maintain consistency:
@Action(providerActions.GetFilteredOfficialEmployees) -getFilteredOfficialEmployees( +getFilteredOfficials( { patchState }: StateContext<ProviderStateModel>, { payload }: providerActions.GetFilteredOfficialEmployees ): Observable<SearchResponse<OfficialEmployee[]>> { patchState({ isLoading: true }); return this.employeeService .getFilteredOfficialEmployees(payload) .pipe( tap((officials: SearchResponse<OfficialEmployee[]>) => patchState({ officialEmployees: officials ?? EMPTY_RESULT, isLoading: false }) ) ); }
🧹 Nitpick comments (8)
src/app/shared/models/official-employee.model.ts (2)
16-17: Consider using Date type for date fields.The
activeFromandactiveTofields are currently typed as strings. Consider using theDatetype for better type safety and date manipulation.- activeFrom: string; - activeTo: string; + activeFrom: Date; + activeTo: Date;
3-18: Add JSDoc documentation for the class and its properties.Consider adding documentation to describe the purpose of the class and its properties, especially for fields like
rnokpp,dismissalOrder, etc., to improve code maintainability.+/** + * Represents an official employee with employment-related information. + */ export class OfficialEmployee implements Person { + /** Unique identifier for the official employee */ id?: string; + /** Associated user identifier */ userId?: string; // Add documentation for other propertiessrc/app/shared/services/employee/employee.service.ts (2)
24-27: Consider separating official employee operations into a dedicated service.The service is mixing regular employee and official employee operations. Consider creating a separate
OfficialEmployeeServicefor better separation of concerns and maintainability.Also applies to: 33-33
24-26: Update method documentation to reflect official employees.The documentation comment still refers to "provider admins" which seems outdated.
- * This method get provider admisn with filter parameters + * This method retrieves filtered official employees for a provider + * @param filterParams EmployeeParameters containing provider ID and filter criteria + * @returns Observable<SearchResponse<OfficialEmployee[]>>src/app/shared/services/employee/employee.service.spec.ts (1)
31-43: Consider centralizing test data and adding edge cases.The test data is well-structured but could be improved:
- Move mock data to a separate test utilities file for reuse
- Add edge cases for error scenarios (e.g., network errors, invalid responses)
Example test for error handling:
it('should handle error when getting filtered official employees', () => { const filterParams: EmployeeParameters = { searchString: 'John', from: 0, size: 10, providerId: '1' }; service.getFilteredOfficialEmployees(filterParams).subscribe( () => fail('should have failed'), (error) => { expect(error.status).toBe(404); } ); const req = httpMock.expectOne(`/api/v1/providers/${filterParams.providerId}/officials/Get?searchString=John&from=0&size=10`); req.flush('Not Found', { status: 404, statusText: 'Not Found' }); });Also applies to: 45-81
src/app/shell/personal-cabinet/provider/employees/employees.component.ts (2)
34-39: Consider splitting component responsibilities.The component handles too many responsibilities (filtering, pagination, CRUD operations). Consider splitting it into smaller components:
- OfficialEmployeeListComponent
- OfficialEmployeeFiltersComponent
- OfficialEmployeePaginationComponent
193-206: Optimize subscription management.The
addEmployeesSubscriptionsmethod could be simplified using RxJS operators:private addEmployeesSubscriptions(): void { - this.filterFormControl.valueChanges - .pipe( - debounceTime(500), - distinctUntilChanged(), - takeUntil(this.destroy$) - ) - .subscribe((val: string) => { - this.filterParams.searchString = val; - this.currentPage = PaginationConstants.firstPage; - this.getFilteredOfficialEmployees(); - }); + this.filterFormControl.valueChanges.pipe( + debounceTime(500), + distinctUntilChanged(), + tap((val: string) => { + this.filterParams.searchString = val; + this.currentPage = PaginationConstants.firstPage; + }), + tap(() => this.getFilteredOfficialEmployees()), + takeUntil(this.destroy$) + ).subscribe(); this.officialEmployees$.pipe( filter(Boolean), + map((officials: SearchResponse<OfficialEmployee[]>) => ({ + ...officials, + officialEmployeesData: Util.updateStructureForTheTableOfficialEmployees(officials.entities) + })), takeUntil(this.destroy$) ).subscribe((officials: SearchResponse<OfficialEmployee[]>) => { - this.officialEmployees = officials; - this.officialEmployeesData = Util.updateStructureForTheTableOfficialEmployees(officials.entities); + this.officialEmployees = officials; + this.officialEmployeesData = officials.officialEmployeesData; }); }src/app/shared/utils/utils.ts (1)
139-155: Consider adding error handling and input validation.The method looks good but could benefit from defensive programming practices.
Apply this diff to add input validation and error handling:
public static updateStructureForTheTableOfficialEmployees(admins: OfficialEmployee[]): OfficialEmployeeTableData[] { + if (!admins || !Array.isArray(admins)) { + return []; + } const updatedOfficialEmployees: OfficialEmployeeTableData[] = []; admins.forEach((admin: OfficialEmployee) => { + if (!admin) { + return; + } updatedOfficialEmployees.push({ id: admin.id, pib: `${admin.lastName} ${admin.firstName} ${admin.middleName}`, role: admin.position, rnokpp: admin.rnokpp }); }); return updatedOfficialEmployees; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/app/shared/models/official-employee.model.ts(1 hunks)src/app/shared/models/users-table.ts(1 hunks)src/app/shared/services/employee/employee.service.spec.ts(1 hunks)src/app/shared/services/employee/employee.service.ts(2 hunks)src/app/shared/store/provider.actions.ts(1 hunks)src/app/shared/store/provider.state.ts(7 hunks)src/app/shared/utils/utils.ts(2 hunks)src/app/shell/personal-cabinet/provider/employees/employees.component.html(4 hunks)src/app/shell/personal-cabinet/provider/employees/employees.component.spec.ts(5 hunks)src/app/shell/personal-cabinet/provider/employees/employees.component.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/app/shared/models/users-table.ts
- src/app/shared/store/provider.actions.ts
- src/app/shell/personal-cabinet/provider/employees/employees.component.spec.ts
- src/app/shell/personal-cabinet/provider/employees/employees.component.html
🔇 Additional comments (4)
src/app/shared/utils/utils.ts (1)
19-19: LGTM!The imports are well-organized and follow the project's import structure pattern.
Also applies to: 23-23
src/app/shared/store/provider.state.ts (3)
41-41: LGTM!The model property is well-typed and follows the state model pattern.
105-108: LGTM!The selector is correctly implemented and follows the established pattern.
603-603: LGTM!The action updates maintain consistency with the new official employees pattern.
Also applies to: 630-630
| private getFilteredOfficialEmployees(): void { | ||
| Util.setFromPaginationParam(this.filterParams, this.currentPage, this.officialEmployees?.totalAmount); | ||
| this.store.dispatch(new GetFilteredOfficialEmployees(this.filterParams)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for failed API calls.
The getFilteredOfficialEmployees method should handle potential API failures and show appropriate error messages to users.
private getFilteredOfficialEmployees(): void {
Util.setFromPaginationParam(this.filterParams, this.currentPage, this.officialEmployees?.totalAmount);
- this.store.dispatch(new GetFilteredOfficialEmployees(this.filterParams));
+ this.store.dispatch(new GetFilteredOfficialEmployees(this.filterParams))
+ .pipe(
+ catchError(error => {
+ // Show error message to user
+ console.error('Failed to fetch official employees:', error);
+ return EMPTY;
+ })
+ );
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
@humanmantis Understood, I'll skip the error handling suggestion.
(｡•́︿•̀｡)
| @@ -1,3 +1,4 @@ | |||
| import { GetFilteredOfficials } from 'shared/store/provider.actions'; | |||
| dismissalOrder: string; | ||
| recruitmentOrder: string; | ||
| dismissalReason: string; | ||
| employmentType: string; |
There was a problem hiding this comment.
Probably should be an enum instead
There was a problem hiding this comment.
Dima said these are some new "placeholder" fields which can be removed or edited in the future, maybe it is better to leave them like that
There was a problem hiding this comment.
I asked him and we decided just to ignore it and add TODO comment
YakymivY
force-pushed
the
Yakymiv/officials-list
branch
from
77120e2 to
c421437
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/app/shared/utils/utils.ts (2)
149-149: Consider reusing thegetFullNameutility method.For consistency with other methods in this file, consider using the existing
getFullNameutility method:- pib: `${admin.lastName} ${admin.firstName} ${admin.middleName}`, + pib: Util.getFullName(admin),
144-155: Add input validation for null safety.Consider adding null checks to prevent runtime errors when processing invalid input:
public static updateStructureForTheTableOfficialEmployees(admins: OfficialEmployee[]): OfficialEmployeeTableData[] { + if (!admins?.length) { + return []; + } const updatedOfficialEmployees: OfficialEmployeeTableData[] = []; admins.forEach((admin: OfficialEmployee) => { + if (!admin) { + return; + } updatedOfficialEmployees.push({ id: admin.id, pib: Util.getFullName(admin), role: admin.position, rnokpp: admin.rnokpp }); }); return updatedOfficialEmployees; }src/app/shared/services/employee/employee.service.spec.ts (1)
149-150: Fix typo in method name.The method name
reinvateEmployeehas a typo and should bereinviteEmployee.- service.reinvateEmployee(employee).subscribe((response) => { + service.reinviteEmployee(employee).subscribe((response) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
src/assets/i18n/en.jsonis excluded by!src/assets/**and included bysrc/**src/assets/i18n/uk.jsonis excluded by!src/assets/**and included bysrc/**
📒 Files selected for processing (13)
src/app/shared/components/users-list/users-list.component.html(1 hunks)src/app/shared/models/employee.model.ts(1 hunks)src/app/shared/models/official-employee.model.ts(1 hunks)src/app/shared/models/users-table.ts(1 hunks)src/app/shared/services/employee/employee.service.spec.ts(1 hunks)src/app/shared/services/employee/employee.service.ts(2 hunks)src/app/shared/store/provider.actions.ts(1 hunks)src/app/shared/store/provider.state.ts(7 hunks)src/app/shared/utils/utils.ts(2 hunks)src/app/shell/personal-cabinet/provider/employees/employees.component.html(4 hunks)src/app/shell/personal-cabinet/provider/employees/employees.component.scss(1 hunks)src/app/shell/personal-cabinet/provider/employees/employees.component.spec.ts(4 hunks)src/app/shell/personal-cabinet/provider/employees/employees.component.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- src/app/shared/models/users-table.ts
- src/app/shared/components/users-list/users-list.component.html
- src/app/shell/personal-cabinet/provider/employees/employees.component.scss
- src/app/shared/store/provider.actions.ts
- src/app/shared/models/official-employee.model.ts
- src/app/shared/services/employee/employee.service.ts
- src/app/shared/models/employee.model.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build_and_test
- GitHub Check: SonarCloud
🔇 Additional comments (25)
src/app/shared/utils/utils.ts (1)
19-19: LGTM! Import changes align with PR objectives.The new imports correctly support the transition to official employee data handling.
Also applies to: 23-23
src/app/shared/services/employee/employee.service.spec.ts (7)
1-25: LGTM! Well-structured test setup.The test setup follows best practices with proper imports, module configuration, and HTTP testing controller setup.
31-43: LGTM! Comprehensive test for GET employee by ID.The test case properly verifies the HTTP method, endpoint, and response handling.
45-81: LGTM! Thorough test for GET filtered official employees.The test case includes comprehensive mock data and verifies the correct endpoint construction with query parameters.
83-95: LGTM! Well-structured test for employee creation.The test case properly verifies both the request body and response handling.
97-109: LGTM! Clear test for employee deletion.The test case properly verifies the DELETE request and query parameters.
111-129: LGTM! Comprehensive test for employee blocking.The test case thoroughly verifies the block operation including request parameters.
131-144: LGTM! Well-structured test for employee update.The test case properly verifies both the request body and response handling.
src/app/shell/personal-cabinet/provider/employees/employees.component.ts (4)
38-38: LGTM! Clear state selector for official employees.The selector is properly typed with the new
OfficialEmployeemodel.
46-47: LGTM! Well-structured data properties.The properties are properly typed for official employees data management.
51-57: LGTM! Clear filter parameters setup.The filter parameters include the new providerId field and proper pagination defaults.
185-188: Add error handling for failed API calls.The
getFilteredOfficialEmployeesmethod should handle potential API failures and show appropriate error messages to users.src/app/shell/personal-cabinet/provider/employees/employees.component.spec.ts (3)
84-92: LGTM! Clear test for page change handling.The test properly verifies both the page update and the subsequent data fetch.
94-102: LGTM! Comprehensive test for items per page change.The test verifies both the size update and the first page load trigger.
206-215: LGTM! Well-structured mock component.The mock component properly defines all required inputs for testing.
src/app/shared/store/provider.state.ts (3)
35-48: LGTM! Clear state model definition.The state model properly includes the officialEmployees property with correct typing.
50-66: LGTM! Well-structured state defaults.The default state properly initializes all required properties including officialEmployees.
326-339: LGTM! Comprehensive official employees action.The action properly handles the state update and loading states.
src/app/shell/personal-cabinet/provider/employees/employees.component.html (7)
2-2: Updated Container Class Name.
The div’s class name is nowsearch-container, which clearly reflects its purpose. Please ensure the corresponding CSS rules are updated to maintain the intended styling.
11-12: Refined Search Texts.
The search input’splaceholderandmatTooltiptext have been updated to"FORMS.PLACEHOLDERS.SEARCH_NAME_RNOKPP_POSITION", aligning with the new official employees criteria. Verify that the translation files include this key.
19-19: Updated Template Data Context.
The*ngTemplateOutletnow passesofficialEmployeesDatato theUsersListtemplate. This change is consistent with the shift from general employees to official employees.Ensure that the TypeScript code initializes and maintains
officialEmployeesDatacorrectly.
23-24: Consistent Data Loading Check.
Within theUsersListtemplate, the condition now checks!!officialEmployeesafter confirming that the loading state (isLoadingCabinet$) is false. This update supports the new official employees workflow.
31-31: Introduction of Displayed Columns Input.
The new[displayedColumns]input on the<app-users-list>component enhances configurability. Verify that theapp-users-listcomponent is updated to accept and correctly utilize this property.
40-40: Updated Paginator Entity Count.
The paginator’stotalEntitiesis now bound toofficialEmployees.totalAmount, ensuring that pagination reflects the updated data source. Confirm thattotalAmountis correctly maintained in the component’s logic.
63-63: Consistent Data Context in Deputy Template.
In theDeputyTableViewTmpl, the template outlet now usesofficialEmployeesData—ensuring both deputy and provider views are aligned in presenting official employee data.
* fetched data from endpoint and tabs removed * placeholder changed and deputy table template removed * employees tests adjusted * unit tests for employee service and component * naming updated to officialEmployee * imports sorted and comment in the model --------- Co-authored-by: Yuriy Yakymiv <yuriyyakymiv@Yuriys-MacBook-Air.local>
changed endpoint, added Official model and adjusted naming
Summary by CodeRabbit
New Features
Style