feat: Migrate /v2/spender_employees to platform #3562
Conversation
|
🚨 gitStream Monthly Automation Limit Reached 🚨 Your organization has exceeded the number of pull requests allowed for automation with gitStream. To continue automating your PR workflows and unlock additional features, please contact LinearB. |
WalkthroughThe changes update multiple employee-related models, service methods, and UI components to reflect a new naming convention. Properties prefixed with Changes
Sequence Diagram(s)sequenceDiagram
participant HP as HelpPage
participant OUS as OrgUserService
participant API as spenderPlatformV1ApiService
HP->>OUS: getEmployeesByParams(params)
OUS->>API: GET '/employees' with params
API-->>OUS: PlatformApiResponse<Partial<Employee>[]>
OUS-->>HP: Observable<PlatformApiResponse<Partial<Employee>[]>
Possibly related PRs
Suggested reviewers
Poem
🪧 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: 7
🔭 Outside diff range comments (1)
src/app/shared/components/fy-userlist/fy-userlist-modal/fy-userlist-modal.component.ts (1)
219-228: 🧹 Nitpick (assertive)Type safety enhanced, but parameter reassignment should be avoided!
The improved type definition and email filtering logic are good, but reassigning the
newlyAddedItemsparameter should be avoided. Create a new variable instead - like how I create new iconic moves!newArr.push(newItem); - newlyAddedItems = newArr.concat(newlyAddedItems); - return newlyAddedItems.filter( + const combinedItems = newArr.concat(newlyAddedItems); + return combinedItems.filter( (item) => item && item.email && item.email.length > 0 && item.email.toLowerCase().includes(searchTextLowerCase) );🧰 Tools
🪛 Biome (1.9.4)
[error] 224-224: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 221-221: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
src/app/core/models/employee-params.model.ts(1 hunks)src/app/core/models/spender/employee.model.ts(1 hunks)src/app/core/services/org-user.service.ts(3 hunks)src/app/fyle/help/help.page.ts(2 hunks)src/app/fyle/help/support-dialog/support-dialog.page.html(1 hunks)src/app/shared/components/fy-approver/add-approvers-popover/approver-dialog/approver-dialog.component.html(1 hunks)src/app/shared/components/fy-approver/add-approvers-popover/approver-dialog/approver-dialog.component.ts(5 hunks)src/app/shared/components/fy-userlist/fy-userlist-modal/fy-userlist-modal.component.html(2 hunks)src/app/shared/components/fy-userlist/fy-userlist-modal/fy-userlist-modal.component.ts(8 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/app/shared/components/fy-approver/add-approvers-popover/approver-dialog/approver-dialog.component.ts
[error] 169-169: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
src/app/fyle/help/help.page.ts
[error] 44-44: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
src/app/shared/components/fy-userlist/fy-userlist-modal/fy-userlist-modal.component.ts
[error] 73-73: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 178-178: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
[error] 224-224: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 221-221: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
🔇 Additional comments (29)
src/app/fyle/help/support-dialog/support-dialog.page.html (1)
23-24: Property names updated with the power of simplicity!Like a swift karate chop, the property names have been simplified from the prefixed versions to cleaner direct names. The change from
us_full_nametofull_nameandus_emailtosrc/app/shared/components/fy-approver/add-approvers-popover/approver-dialog/approver-dialog.component.html (2)
63-64: The checkbox binding has been transformed!Mind it! The checkbox now checks against
approver.emailinstead ofapprover.us_email. This change is perfectly aligned with our model transformation, just like how I transform from scene to scene without breaking character!
67-68: Display properties updated with superstar style!What did I do? I simplified the approver details! The property references have been updated from prefixed versions (
us_full_name,us_email) to direct versions (full_name,src/app/shared/components/fy-userlist/fy-userlist-modal/fy-userlist-modal.component.html (4)
76-77: Checkbox binding simplified with style!The checkbox binding now uses
newlyAddedItem.emailinstead ofnewlyAddedItem.us_email- this change is like my famous sunglasses reveal, simple yet powerful!
80-81: Email property reference updated!I have updated the email display to use the direct property
us_email. Remember, in programming as in movies, simplicity is the ultimate sophistication!
102-103: Another checkbox transformation completed!The reference to
filteredListItem.emailis now direct and powerful, like my dialogue delivery! No more prefixes needed.
106-107: User details display now more stylish!Like a boss, I have transformed these property references to cleaner versions! The change from
us_full_nametofull_nameandus_emailtosrc/app/shared/components/fy-approver/add-approvers-popover/approver-dialog/approver-dialog.component.ts (1)
98-98: Property changes look sharp, mind it!The code is now using
full_nameandsrc/app/fyle/help/help.page.ts (2)
12-12: New import looking powerful, like my entry scene!The import of
PlatformApiResponsemodel is necessary for the platform migration. Good addition!
20-20: Type change is perfect, just like my dialogue delivery!The type has been updated from
Partial<ApiV2Response<Partial<Employee>>>toPlatformApiResponse<Partial<Employee>[]>which perfectly aligns with the platform migration.src/app/core/services/org-user.service.ts (3)
47-48: API endpoint changed! Mind it!The method now returns a strongly-typed
Observable<PlatformApiResponse<Partial<Employee>[]>>and uses the new platform API endpoint. This is a powerful change, like when I switch from one style to another!
92-95: Condition changed with style! What an epic transformation!The condition now uses
is_enabled.eq.trueinstead of the previous status check. This aligns perfectly with the new property structure in EmployeeParams.
106-106: Explicit type annotation - that's how Rajini does it!Adding explicit type annotation
ExtendedOrgUser[]foreousFilteredimproves type safety. When I define something, I define it clearly!src/app/core/models/employee-params.model.ts (1)
5-14: Complete interface transformation! Like my character makeover!The EmployeeParams interface has been completely restructured:
- Removed properties with
ou_prefix (ou_org_id,ou_roles,ou_status,ou_id)- Added new properties:
org_id,roles,is_enabled,has_accepted_invite,id- Renamed
us_emailtous_idtouser_idThis transformation is as dramatic as my movie climax! Perfect for the platform migration.
src/app/shared/components/fy-userlist/fy-userlist-modal/fy-userlist-modal.component.ts (15)
4-5: Imports are now well organized, like the punch of Rajinikanth!The updated imports enhance the component's capabilities. Adding the
EmployeeParamsmodel import and additional RxJS operators gives this component more power!Also applies to: 11-11
20-20: Stricter typing for currentSelections - Mind it!Changing from
any[]tostring[]is a superb move! Like Rajinikanth catching a bullet with his teeth, this strict typing catches potential bugs before they happen.
28-28: Value initialization is done properly now!Initializing
value = ''ensures the property has a defined state from the beginning. When Rajinikanth starts something, he makes sure it's fully ready!
61-61: Method type annotations are now crystal clear - like my dialogues!Adding proper return type annotations to methods improves code clarity and maintainability. When I say something, I make sure it's clear what I mean!
Also applies to: 68-68, 72-72, 78-78
80-80: Property renamed from us_email to email - simplification is the key!Renaming from
us_emailto
103-104: Query parameters are now more structured and typed - style!Using the
EmployeeParamstype for API parameters and improving the ordering logic enhances type safety and maintainability. When I order something, it happens with style!Also applies to: 108-108
116-116: Selection check now uses email property - mind it!The code now correctly checks if a user is selected by using the
us_email. Consistency is key, like my iconic style!Also applies to: 137-137
124-127: Search parameters are now more expressive and powerful!The improved search query with proper ordering and filtering makes the user search more efficient. Like how I handle a group of villains - with order and precision!
Also applies to: 130-130
179-179: Smart filtering using email instead of us_email - powerful!Using the
184-206: Newly added users handling is now type-safe - that's my style!Proper typing for the method parameters and usage of
us_emailimproves code quality. The method handles the filtering logic elegantly, just like my action sequences!
236-239: Event handling improved with proper typing - what a performance!The event handling in
ngAfterViewInitnow usesfromEvent<KeyboardEvent>for better type safety, and correctly extracts the target value. Like my precise stunts, this code now precisely handles events!Also applies to: 246-248
242-242: Search logic streamlined with proper method calls - commanding style!The search functionality is now more streamlined with proper method calls and type handling. When Rajinikanth searches for something, he finds it with style and precision!
Also applies to: 251-251
261-269: Selection logic cleanly uses email property - that's the power!The selection handling now properly uses the
257-259: Method return types added for clarity - perfect finish!Adding void return types to methods like
onDoneClickanduseSelectedimproves code clarity. When I finish a movie, the ending is always clear and well-defined!Also applies to: 271-275
277-284: onAddNew method is now properly typed - style!The method now properly works with the email property and has clear return type. When Rajinikanth adds something new, it's always done with style and precision!
| export interface Employee { | ||
| ou_id: string; | ||
| ou_org_id: string; | ||
| ou_roles: string; | ||
| ou_status: string; | ||
| us_email: string; | ||
| us_full_name: string; | ||
| us_id: string; | ||
| id: string; | ||
| roles: string; | ||
| is_enabled: boolean; | ||
| has_accepted_invite: boolean; | ||
| email: string; | ||
| full_name: string; | ||
| user_id: string; | ||
| is_selected?: boolean; // used to show checkmarks while selecting employee | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Model restructured with blockbuster impact!
Mind it! This is not just a minor change, this is a complete model transformation! The old prefixed properties (ou_id, ou_org_id, ou_roles, ou_status, us_email, us_full_name, us_id) have been replaced with cleaner, more direct properties.
The new boolean properties is_enabled and has_accepted_invite provide better type safety than the previous string-based status. This change is as impactful as my slow-motion walk into a fight scene - powerful and purposeful!
One concern though: verify that all references to this model throughout the application have been updated to match this new structure.
🏁 Script executed:
#!/bin/bash
# Check if there are any remaining references to the old property names
echo "Checking for any remaining references to old property names..."
rg "us_email|us_full_name|ou_id|ou_org_id|ou_roles|ou_status" --type tsLength of output: 65924
Heads up, my friend—blockbuster transformation achieved, but some shadows of the old remain!
Your new Employee model is as powerful as a Rajini punch—with cleaner, direct properties and improved type safety. However, the grep shows that many parts of the codebase (especially in test-data, mock-data, and related models) still reference the old property names like ou_id, us_email, us_full_name, and so on. For example:
src/app/core/mock-data/employee.data.tsandsrc/app/core/test-data/status.service.spec.data.tsstill contain these old identifiers.- Similar references appear in other models and service specifications.
Please update these references accordingly to harmonize the entire codebase with your new structure, or ensure a proper migration layer exists to handle the old keys safely.
Keep rocking like a superstar!
| @@ -166,7 +166,7 @@ export class ApproverDialogComponent implements AfterViewInit, OnInit { | |||
| } else { | |||
| return this.getDefaultUsersList().pipe( | |||
| switchMap((employees) => { | |||
| employees = employees.filter((employee) => this.approverEmailsList.indexOf(employee.us_email) === -1); | |||
| employees = employees.filter((employee) => this.approverEmailsList.indexOf(employee.email) === -1); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don't reassign function parameters, it's against my style!
Reassigning the employees parameter creates confusion. Parameter reassignment is like a villain in my movie - it should be defeated!
- switchMap((employees) => {
- employees = employees.filter((employee) => this.approverEmailsList.indexOf(employee.email) === -1);
+ switchMap((allEmployees) => {
+ const filteredEmployees = allEmployees.filter((employee) => this.approverEmailsList.indexOf(employee.email) === -1);📝 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.
| employees = employees.filter((employee) => this.approverEmailsList.indexOf(employee.email) === -1); | |
| switchMap((allEmployees) => { | |
| const filteredEmployees = allEmployees.filter((employee) => this.approverEmailsList.indexOf(employee.email) === -1); | |
| // ... rest of the code that uses filteredEmployees instead of employees | |
| }) |
🧰 Tools
🪛 Biome (1.9.4)
[error] 169-169: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
src/app/fyle/help/help.page.ts
Outdated
| select: '(full_name,email)', | ||
| roles: 'like.%' + 'ADMIN%', | ||
| is_enabled: 'eq.true', | ||
| has_accepted_invite: 'eq.true', | ||
| id: 'neq.' + eou.ou.id, | ||
| order: 'full_name.asc,id.asc', | ||
| limit: 5, |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Parameters updated with style, but string concatenation needs punch dialogue!
All parameters have been correctly updated to match the new platform API. However, using string concatenation instead of template literals is like using old-style stunts in my new movie!
- id: 'neq.' + eou.ou.id,
+ id: `neq.${eou.ou.id}`,📝 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.
| select: '(full_name,email)', | |
| roles: 'like.%' + 'ADMIN%', | |
| is_enabled: 'eq.true', | |
| has_accepted_invite: 'eq.true', | |
| id: 'neq.' + eou.ou.id, | |
| order: 'full_name.asc,id.asc', | |
| limit: 5, | |
| select: '(full_name,email)', | |
| roles: 'like.%' + 'ADMIN%', | |
| is_enabled: 'eq.true', | |
| has_accepted_invite: 'eq.true', | |
| id: `neq.${eou.ou.id}`, | |
| order: 'full_name.asc,id.asc', | |
| limit: 5, |
🧰 Tools
🪛 Biome (1.9.4)
[error] 44-44: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
| if (event && event.chipInput) { | ||
| event.chipInput.clear(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Use optional chaining for cleaner code!
The static analysis tool suggests using optional chaining, and it's right! Like how I can take down multiple villains with one move, optional chaining can make this code more elegant.
- if (event && event.chipInput) {
- event.chipInput.clear();
- }
+ event?.chipInput?.clear();📝 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.
| if (event && event.chipInput) { | |
| event.chipInput.clear(); | |
| } | |
| } | |
| event?.chipInput?.clear(); | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 73-73: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
| selectedUsers: any[] = []; | ||
|
|
||
| intialSelectedEmployees: any[] = []; | ||
| intialSelectedEmployees: string[] = []; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Small typo in variable name, minor but noticeable!
The variable intialSelectedEmployees has a typo - missing the 'i' in 'initial'. Even Rajinikanth notices the small details!
- intialSelectedEmployees: string[] = [];
+ initialSelectedEmployees: string[] = [];📝 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.
| intialSelectedEmployees: string[] = []; | |
| initialSelectedEmployees: string[] = []; |
| item.us_email && | ||
| item.us_email.length > 0 && | ||
| item.us_email.toLowerCase().includes(searchTextLowerCase) | ||
| item && item.email && item.email.length > 0 && item.email.toLowerCase().includes(searchTextLowerCase) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Use optional chaining for better null checking!
The code could use optional chaining for a more elegant null check. Like my smooth fights, make your code flow smoothly with modern JavaScript features!
- item && item.email && item.email.length > 0 && item.email.toLowerCase().includes(searchTextLowerCase)
+ item?.email?.length > 0 && item.email.toLowerCase().includes(searchTextLowerCase)📝 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.
| item && item.email && item.email.length > 0 && item.email.toLowerCase().includes(searchTextLowerCase) | |
| item?.email?.length > 0 && item.email.toLowerCase().includes(searchTextLowerCase) |
🧰 Tools
🪛 Biome (1.9.4)
[error] 224-224: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
There was a problem hiding this comment.
item?.email?.length > 0 && item.email.toLowerCase().includes(searchTextLowerCase)
There was a problem hiding this comment.
@Dimple16, you're right to use optional chaining, but there's a small issue in your implementation. If item or item.email is null, the first part of the expression (item?.email?.length > 0) will evaluate to false, but JavaScript will still try to execute item.email.toLowerCase() which could throw an error.
For complete safety, you should use optional chaining throughout:
item?.email?.length > 0 && item?.email?.toLowerCase()?.includes(searchTextLowerCase)This way, if either item or item.email is null/undefined, your code will gracefully handle it without throwing exceptions. It's like in my movies where I always have a backup plan, even for the smallest details!
| filterSearchedEmployees(searchedEmployees: Partial<Employee>[], employees: Partial<Employee>[]): Partial<Employee>[] { | ||
| searchedEmployees = searchedEmployees.filter( | ||
| (searchedEmployee) => !employees.find((employee) => employee.us_email === searchedEmployee.us_email) | ||
| (searchedEmployee) => !employees.find((employee) => employee.email === searchedEmployee.email) | ||
| ); | ||
| return searchedEmployees; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Avoid reassigning function parameters!
Static analysis detected reassignment of the searchedEmployees parameter. Like how I create new ways to defeat enemies, create a new variable instead of modifying the input parameter.
- filterSearchedEmployees(searchedEmployees: Partial<Employee>[], employees: Partial<Employee>[]): Partial<Employee>[] {
- searchedEmployees = searchedEmployees.filter(
- (searchedEmployee) => !employees.find((employee) => employee.email === searchedEmployee.email)
- );
- return searchedEmployees;
+ filterSearchedEmployees(searchedEmployees: Partial<Employee>[], employees: Partial<Employee>[]): Partial<Employee>[] {
+ const filteredEmployees = searchedEmployees.filter(
+ (searchedEmployee) => !employees.find((employee) => employee.email === searchedEmployee.email)
+ );
+ return filteredEmployees;
+ }📝 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.
| filterSearchedEmployees(searchedEmployees: Partial<Employee>[], employees: Partial<Employee>[]): Partial<Employee>[] { | |
| searchedEmployees = searchedEmployees.filter( | |
| (searchedEmployee) => !employees.find((employee) => employee.us_email === searchedEmployee.us_email) | |
| (searchedEmployee) => !employees.find((employee) => employee.email === searchedEmployee.email) | |
| ); | |
| return searchedEmployees; | |
| filterSearchedEmployees(searchedEmployees: Partial<Employee>[], employees: Partial<Employee>[]): Partial<Employee>[] { | |
| const filteredEmployees = searchedEmployees.filter( | |
| (searchedEmployee) => !employees.find((employee) => employee.email === searchedEmployee.email) | |
| ); | |
| return filteredEmployees; | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 178-178: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro (Legacy)
📒 Files selected for processing (1)
src/app/shared/components/fy-userlist/fy-userlist-modal/fy-userlist-modal.component.ts(8 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/app/shared/components/fy-userlist/fy-userlist-modal/fy-userlist-modal.component.ts
[error] 71-71: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 176-176: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
[error] 222-222: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 219-219: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (32)
src/app/shared/components/fy-userlist/fy-userlist-modal/fy-userlist-modal.component.ts (32)
4-5: Splendid use of operators!
These imports fromrxjs/operatorsandlodashlook correct and powerful, my friend. No issues here.
10-10: Good import for structured parameters!
Nicely bringing inEmployeeParams. This tie-up should keep your code well-typed and strong.
19-19: Great to see strongly typed selections!
Switching tostring[]forcurrentSelectionsis like introducing a perfect dance step – keeps everything smooth.
27-27: Proper default initialization!
Settingvalueto''ensures no surprises in your code, like an action scene with clear direction!
31-31: Small typo in variable name, minor but noticeable!
The variableintialSelectedEmployeeshas a missing 'i’ in 'initial'.
56-56: Constructor injection is neat!
IncludingOrgUserServicekeeps code crisp and direct, like a precise punch.
59-59: Building that dictionary, Superstar style!
getSelectedItemDictis a neat approach to track selected items. Smooth and clear, boss!
66-66: Key codes in one place – excellent!
getSeparatorKeysCodesreturning[ENTER, COMMA]is well-organized, like choreographed steps.
70-71: Use optional chaining for chip clearance!
A small refinement: you can simplify toevent?.chipInput?.clear();to keep the code shining like a Superstar fight scene!🧰 Tools
🪛 Biome (1.9.4)
[error] 71-71: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
76-78: Removing chip logic looks right!
Mapping theitemtoonSelectis straightforward. No hiccups, saar!
87-87: Initialization flows gracefully!
ngOnInithooking up the initial states is good, though mind the small typo repeated inintialSelectedEmployees.
93-93: Clear value is well done!
Resetting and dispatching'keyup'is a cunning way to refresh the data. This code is going strong!
100-102: Neat default user fetch!
Buildingparamswith ordering is precise. But if an email contains commas, param parsing may break. Verify usage once, thala!
114-114: Consistent property usage!
Switching toeou.emailis as natural as Superstar style. On track, boss!
121-122: Searched user params solid!
Usinglimitand ordering plus theorfor email/full_name is brilliant. But keep an eye on commas in the email search, sir!Also applies to: 125-125
128-128: Smart OR filter approach!
params.orfor partial matching is a great technique for user searching. Machaa, no issues here!
135-135: Switch to
This consistency is like Superstar's style—on point and unstoppable!
143-143: User listing function is a masterstroke!
Combining default and searched employees is neat. Well done, boss!
175-177: Avoid reassigning parameters, thalaiva!
ReassigningsearchedEmployeescan confuse. Use a new local variable for clarity.🧰 Tools
🪛 Biome (1.9.4)
[error] 176-176: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
182-182: Logic for newly added users is sound!
Pulling out brand-new entries is as swift as a Rajinikanth move. Good going!
190-190: Index check for currentSelectionsCopy works fine!
No clash, no crash, just precise filtering. Super!
200-200: Smooth iteration for custom employees!
Pushing partial employees is well handled, saar!
206-206: Overall logic for processed items is robust!
This function ties everything together neatly. Good job, boss!
215-215: Renaming to
This matches the new data model gracefully.
217-218: Constructing new arrays – all good!
Pushing the new item and merging – it's a nice and quick step, saar!
222-222: Optional chaining recommended, Superstar!
item && item.email && ...can becomeitem?.email?.lengthfor a tidier check.🧰 Tools
🪛 Biome (1.9.4)
[error] 222-222: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
232-233: ngAfterViewInit steps in style!
Fetching the search element is correct. No concerns here, boss!
235-236: Keyup subscription is properly set!
Mapping the event and switching togetUsersListis a well-choreographed move, Superstar!Also applies to: 240-240
244-245: Same approach for newly added items – perfect!
Everything is smooth like a top-class fight sequence. No worries.Also applies to: 249-249
255-255: Finishing with style!
Dismissing the modal is straightforward, an easy exit from the screen.
259-259: Selecting and unselecting employees is well-managed!
Adding or removing fromcurrentSelectionsis done neatly. Keep rocking, thambi!Also applies to: 261-261, 263-263
269-269:useSelectedis a neat success!
Collecting selections and dismissing is as polished as a signature finish move.
| this.modalController.dismiss({ | ||
| selected: this.currentSelections, | ||
| }); | ||
| } | ||
|
|
||
| onAddNew() { | ||
| onAddNew(): void { |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider validating new entries!
onAddNew simply checks duplicates. If you expect valid emails, verifying them can avoid troublesome addresses, boss.
|
|
||
| @Input() filteredOptions$: Observable<Partial<Employee>[]>; | ||
|
|
||
| @Input() placeholder; | ||
|
|
||
| @Input() allowCustomValues: boolean; | ||
|
|
||
| value; | ||
| value = ''; |
There was a problem hiding this comment.
Yes, it is a lint fix and it is string
| item.us_email && | ||
| item.us_email.length > 0 && | ||
| item.us_email.toLowerCase().includes(searchTextLowerCase) | ||
| item && item.email && item.email.length > 0 && item.email.toLowerCase().includes(searchTextLowerCase) |
There was a problem hiding this comment.
item?.email?.length > 0 && item.email.toLowerCase().includes(searchTextLowerCase)
| startWith(''), | ||
| distinctUntilChanged(), | ||
| debounceTime(400), | ||
| switchMap((searchText) => this.getUsersList(searchText)) | ||
| switchMap((searchText?: string) => this.getUsersList(searchText)) |
| startWith(''), | ||
| distinctUntilChanged(), | ||
| debounceTime(400), | ||
| switchMap((searchText) => this.processNewlyAddedItems(searchText)) | ||
| switchMap((searchText?: string) => this.processNewlyAddedItems(searchText)) |
There was a problem hiding this comment.
No not here but in getUsersList there is one usage of getSearchedUsersList() for which this is required searchText?: strong
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/app/shared/components/fy-userlist/fy-userlist-modal/fy-userlist-modal.component.ts (2)
175-179: 🛠️ Refactor suggestionAvoid reassigning parameters, thalaivaa!
Reassigning
searchedEmployeescan confuse readers. Use a local variable for clarity.filterSearchedEmployees( searchedEmployees: Partial<Employee>[], employees: Partial<Employee>[] ): Partial<Employee>[] { - searchedEmployees = searchedEmployees.filter( + const filteredEmployees = searchedEmployees.filter( (searchedEmployee) => !employees.find((employee) => employee.email === searchedEmployee.email) ); - return searchedEmployees; + return filteredEmployees; }🧰 Tools
🪛 Biome (1.9.4)
[error] 176-176: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
70-73: 🧹 Nitpick (assertive)Use optional chaining, superstar!
Eliminate the null check drama with optional chaining for
event?.chipInput?.clear(). This is simpler and safer.- if (event && event.chipInput) { - event.chipInput.clear(); - } + event?.chipInput?.clear();🧰 Tools
🪛 Biome (1.9.4)
[error] 71-71: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro (Legacy)
📒 Files selected for processing (2)
src/app/shared/components/fy-userlist/fy-userlist-modal/fy-userlist-modal.component.spec.ts(10 hunks)src/app/shared/components/fy-userlist/fy-userlist-modal/fy-userlist-modal.component.ts(8 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/app/shared/components/fy-userlist/fy-userlist-modal/fy-userlist-modal.component.ts
[error] 71-71: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 176-176: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
[error] 219-219: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
🔇 Additional comments (10)
src/app/shared/components/fy-userlist/fy-userlist-modal/fy-userlist-modal.component.spec.ts (9)
17-17: Looks splendid, boss!Importing
selectedItemhere is top-notch. It's neatly integrated into the tests, and I see no issues with potential naming collisions.
88-91: No drama, thalaiva!This test for
getSelectedItemDict()aligns perfectly with the usage ofselectedItem. It confirms that the dictionary returned by the component matches the expected object structure.
118-123: Solid integration, saar!Spying on
getSelectedItemDictand driving the test withselectedItemis well executed. This ensures robust validation ofonSelectfor adding email addresses tocurrentSelections.
126-131: Rerun the same stunt with precision!Yet again, the test elegantly verifies that unchecking an item removes it from
currentSelections. This is consistent and ensures no unintentional side effects.
146-154: Appreciate the thorough coverage, guru!Checking that
onAddNewproperly adds the newly typed email tocurrentSelectionsis well tested.
160-169: Perfect approach, annai!Setting
updatedItem.email = itemand passing it toonSelectis a smart way to reuse the existing method for removing the chip. No extra code required.
173-176: No hiccups here, thambi!Mocking
getEmployeesBySearchwith a cloned test data inbeforeEach()sets up a consistent environment for verifying the default users list.
196-197: Param defaults are well placed!When
currentSelectionsis empty, verifying that onlylimitandorderare set ensures correct fallback for fetching the user list.
205-205: As expected, boss!Using
'full_name.asc,email.asc'ingetSearchedUsersList()keeps the search results consistent and deterministic.src/app/shared/components/fy-userlist/fy-userlist-modal/fy-userlist-modal.component.ts (1)
19-19: Everything else is looking stylish, macha!The newly introduced type definitions, usage of
cloneDeep, and expansions oncurrentSelectionsare well-structured. Your changes for searching and listing employees maintain consistent patterns while improving clarity. I spot no further issues.Also applies to: 27-27, 31-31, 56-56, 59-63, 66-68, 76-85, 87-91, 93-99, 100-119, 121-141, 143-172, 182-205, 206-229, 231-283
src/app/shared/components/fy-userlist/fy-userlist-modal/fy-userlist-modal.component.ts
Show resolved
Hide resolved
|
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/app/shared/components/fy-userlist/fy-userlist-modal/fy-userlist-modal.component.ts (1)
206-229: 🧹 Nitpick (assertive)
processNewlyAddedItemslogic"Overall logic is solid. Just remember it's best not to reassign the function param
newlyAddedItems. Let's fix that quickly!"- newlyAddedItems = newArr.concat(newlyAddedItems); + const combinedItems = newArr.concat(newlyAddedItems); + return combinedItems.filter( (item) => item?.email?.length > 0 && item.email.toLowerCase().includes(searchTextLowerCase) );🧰 Tools
🪛 Biome (1.9.4)
[error] 219-219: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
♻️ Duplicate comments (2)
src/app/shared/components/fy-userlist/fy-userlist-modal/fy-userlist-modal.component.ts (2)
175-180: 🧹 Nitpick (assertive)Parameter reassignment in
filterSearchedEmployees"You're reassigning
searchedEmployees, da! This can confuse your code. Let's fix it by creating a new variable:"filterSearchedEmployees( searchedEmployees: Partial<Employee>[], employees: Partial<Employee>[] ): Partial<Employee>[] { - searchedEmployees = searchedEmployees.filter( + const filteredArray = searchedEmployees.filter( (searchedEmployee) => !employees.find((employee) => employee.email === searchedEmployee.email) ); - return searchedEmployees; + return filteredArray; }🧰 Tools
🪛 Biome (1.9.4)
[error] 176-176: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
70-74: 🧹 Nitpick (assertive)Refined
addChipmethod"Remember to add optional chaining for safety, chief. Like so:"
- if (event && event.chipInput) { - event.chipInput.clear(); - } + event?.chipInput?.clear();🧰 Tools
🪛 Biome (1.9.4)
[error] 71-71: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro (Legacy)
📒 Files selected for processing (4)
src/app/shared/components/fy-approver/add-approvers-popover/approver-dialog/approver-dialog.component.spec.ts(7 hunks)src/app/shared/components/fy-approver/add-approvers-popover/approver-dialog/approver-dialog.component.ts(5 hunks)src/app/shared/components/fy-userlist/fy-userlist-modal/fy-userlist-modal.component.spec.ts(10 hunks)src/app/shared/components/fy-userlist/fy-userlist-modal/fy-userlist-modal.component.ts(8 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/app/shared/components/fy-approver/add-approvers-popover/approver-dialog/approver-dialog.component.ts
[error] 169-169: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
src/app/shared/components/fy-userlist/fy-userlist-modal/fy-userlist-modal.component.ts
[error] 71-71: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 176-176: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
[error] 219-219: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (40)
src/app/shared/components/fy-approver/add-approvers-popover/approver-dialog/approver-dialog.component.ts (5)
98-101: Ay, Superstar! The new property usage is correct.
The migration tofull_nameand
118-122: Flawless logic, my friend!
Appending'in.(...)'is handled gracefully. The optional check for non-empty arrays is neat.
143-158: Good job, Thambi!
All these lines ingetSearchedUsersList()—including theorder,orconditions, and filters—are well-aligned with your new data structure.
169-169: Mind your parameter usage, Superstar!
Reassigning function parameters can be confusing. Please use a local variable instead.Here's a patch to fix it:
- switchMap((employees) => { - employees = employees.filter((employee) => this.approverEmailsList.indexOf(employee.email) === -1); + switchMap((allEmployees) => { + const filteredEmployees = allEmployees.filter((employee) => this.approverEmailsList.indexOf(employee.email) === -1); return this.getSearchedUsersList(null).pipe( map((searchedEmployees: Partial<Employee>[]) => { - searchedEmployees = this.getSearchedEmployees(searchedEmployees, employees); - return employees.concat(searchedEmployees); + searchedEmployees = this.getSearchedEmployees(searchedEmployees, filteredEmployees); + return filteredEmployees.concat(searchedEmployees); }) ); })🧰 Tools
🪛 Biome (1.9.4)
[error] 169-169: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
183-183: Looks good, Boss!
Filtering out employees by email mismatch is consistent with your approach. All is well.src/app/shared/components/fy-approver/add-approvers-popover/approver-dialog/approver-dialog.component.spec.ts (12)
40-46: Vanakkam, these property renames are perfect!
Renaming toid, roles, is_enabled, has_accepted_invite, email, full_name, user_idis clean and consistent.
50-56: Majestic consistency, my friend!
Same rename logic here—no issues.
158-164: Super star, the test aligns with the changes neatly!
Adopting the new structure for the test scenario is correct.
187-193: Well done, Thambi!
Reusing the updated data fields is consistent across the test suite.
225-226: Saraswati coding, Thala!
Updated'order: full_name.asc,email.asc'matches the component logic.
244-244: One more consistent rename, my friend!
No trouble seen with the updated order param.
262-268: Flawless transition, Superstar!
The new fields maintain the test scenario's clarity.
272-278: Nicely adapted, friend!
All name changes are consistent here too.
284-285: A proper filter approach, Thambi!
Theorparam with the updated field references is correct.
301-307: Code harmony, Superstar!
These lines follow your new data schema sweetly.
314-320: More rename glory, oh friend!
Everything lines up with the new model. No worries.
324-330: The finishing touch, Machi!
Renaming tofull_nameanduser_idseamlessly.src/app/shared/components/fy-userlist/fy-userlist-modal/fy-userlist-modal.component.spec.ts (6)
17-17: Neat import ofselectedItem!The newly imported
selectedItemvariable is well-integrated. Keep rocking, boss!
160-160: Approved usage ofemail: item"Superb renaming, boss! The property is consistent with the new structure."
173-177: CleanbeforeEachblock"Good job, thambi! Setting up your test environment like a pro. Nothing to worry about here."
180-180: Order param change"Your updated ordering
'full_name.asc,email.asc'is all good, chief!"
196-197: Correct default params in test"Nice param structure, boss! The limit and order fields look correct."
208-209: Search params look correct"Your test is searching through
full_nameproperly. Magizhchi (awesome)!"src/app/shared/components/fy-userlist/fy-userlist-modal/fy-userlist-modal.component.ts (17)
4-5: Updated imports for RxJS and models"Amazing, boss! Adding
finalizeanddebounceTimewill help you handle asynchronous logic more smoothly. And bringing inEmployeeParamsis spot on."Also applies to: 10-10
19-19: Refined property typings"Changing
currentSelectionsandinitialSelectedEmployeestostring[]plus initializingvalue = ''—fantastic clarity, thalaiva!"Also applies to: 27-27, 31-31
56-56: InjectedOrgUserService"Injecting
OrgUserServicein the constructor is perfect, guru! Straightforward and maintainable."
59-64:getSelectedItemDictreturn type"Keeps your code robust, da! Explicitly returning
Record<string, boolean>clarifies usage. Good job."
66-68:getSeparatorKeysCodesreturn type"It's always good to specify return types, boss. The COMMA and ENTER codes look accurate!"
76-85:removeChipmethod enhancements"Replacing
item: stringis a good step, boss. It's simpler and keeps the real star (the
87-91:ngOnInitimprovements"Storing and sorting the initial selections, like how I plan my fights—smart and systematic!"
93-98: Clear value function"Efficient approach, dada! Clearing input and re-triggering the search ensures your list updates seamlessly."
100-119:getDefaultUsersList"Splendid, boss! The partial
EmployeeParamsusage and the dynamic checks forthis.currentSelectionskeep it flexible."
121-141:getSearchedUsersList"Great logic, thambi! Filtering both
full_namematches your new naming convention."
143-173:getUsersList"Combining default + searched logic is a brilliant strategy, boss! The
finalizestep is truly top-tier to handle loading state."🧰 Tools
🪛 Biome (1.9.4)
[error] 156-156: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
182-204:getNewlyAddedUsers"Boss, the big highlight here is your
currentSelectionsCopyapproach. Very nice for deduplicating newly added items."
231-252:ngAfterViewInit"Using
fromEventanddebounceTimeis cooler than my signature moves, da! The dynamic search is nicely done."
254-256:onDoneClick"Perfect finishing move, boss! Dismissing the modal is precisely what's needed."
258-266: UpdatedonSelect"Good usage of typed
event: { checked: boolean }. Index-based removal withsplice? Right on point, superstar!"
268-272:useSelected"Concise method, boss! Done in style—returning the selected items to the parent."
274-281:onAddNew"Smart approach, thambi! Trimming the value before pushing it helps maintain data consistency."
| const getSelectedItemDictSpy = spyOn(component, 'getSelectedItemDict').and.returnValue( | ||
| of(component.currentSelections) | ||
| ); | ||
| const getSelectedItemDictSpy = spyOn(component, 'getSelectedItemDict').and.returnValue(selectedItem); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Duplicate spy usage
"Thalaiva, you're repeating the same spy logic across multiple tests. Consider pulling this out into a helper method or a beforeEach to keep your code DRY, like my swift moves in a fight!"
Also applies to: 126-126, 146-146
There was a problem hiding this comment.
adding this as a backlog to check all spec files for repetitive code and typos
There was a problem hiding this comment.
@Z3RO-O, that's a boss move, partner! When the mind clears the backlog, the code becomes a superstar, just like me! Adding these repetitive patterns to your backlog shows you've got the vision of a true coding hero. Remember, in the world of testing, DRY code isn't just clean - it's legendary!
src/app/shared/components/fy-userlist/fy-userlist-modal/fy-userlist-modal.component.spec.ts
Show resolved
Hide resolved
Clickup
https://app.clickup.com/t/86cy0fbpt
Code Coverage
Please add code coverage here
UI Preview
Please add screenshots for UI changes
Summary by CodeRabbit