feat: add LD flag for restricting actions#4142
Open
harshal015 wants to merge 14 commits intomasterfrom
Open
Conversation
WalkthroughEndra da, this pull request introduces a comprehensive delegation feature across the application. A new DelegationService is created to manage delegatee ownership checks, OrgUserService is enhanced with storage-backed delegation state, and multiple expense and report pages are updated with feature-flagged permission guards. The logout flow now clears delegatee context from storage. Changes
Sequence Diagram(s)sequenceDiagram
participant Page as Expense/Mileage/PerDiem Page
participant LD as LaunchDarkly Service
participant Delegation as Delegation Service
participant OrgUser as OrgUserService
participant Storage as Storage Service
Page->>LD: getVariation('controlled_delegate_access_enabled')
LD-->>Page: false / true
alt Feature Flag Enabled & Team View
Page->>Delegation: isDelegateeOwnedExpense(userId)
Delegation->>OrgUser: isSwitchedToDelegator()
OrgUser-->>Delegation: true/false
alt In Delegatee Mode
Delegation->>Storage: getDelegateeUserId()
Storage-->>Delegation: delegateeId
Delegation-->>Page: ownership check result
else Not in Delegatee Mode
Delegation-->>Page: false
end
Page->>Page: Update delegateeOwnedExpense signal
else Feature Flag Disabled or Not Team View
Page->>Page: Set delegateeOwnedExpense to false
end
Page->>Page: Delete button visibility = !delegateeOwnedExpense && canDelete$
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can generate a title for your PR based on the changes.Add |
🔁 Code Duplication Report - Angular
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/fyle/delegated-accounts/delegated-accounts.page.spec.ts (1)
123-139:⚠️ Potential issue | 🟡 MinorMissing assertion for delegatee context setup before switch.
Line 127 stubs
setDelegateeUserId, but this test never verifies it was called (or called beforeswitchToDelegator). If that call regresses, test still passes.🧪 Suggested test hardening
expect(recentLocalStorageItemsService.clearRecentLocalStorageCache).toHaveBeenCalledTimes(1); + expect(orgUserService.setDelegateeUserId).toHaveBeenCalledOnceWith(eouUnFlattended.us.id); + expect(orgUserService.setDelegateeUserId).toHaveBeenCalledBefore(orgUserService.switchToDelegator); expect(orgUserService.switchToDelegator).toHaveBeenCalledOnceWith(delegatorData.user_id, eouUnFlattended.ou.org_id); expect(router.navigate).toHaveBeenCalledOnceWith(['/', 'enterprise', 'my_dashboard']);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/fyle/delegated-accounts/delegated-accounts.page.spec.ts` around lines 123 - 139, The test stubs orgUserService.setDelegateeUserId but never asserts it was invoked or that it ran before switchToDelegator; add assertions after tick(500) to verify setDelegateeUserId was called once with delegatorData.user_id (expect(orgUserService.setDelegateeUserId).toHaveBeenCalledTimes(1); expect(orgUserService.setDelegateeUserId).toHaveBeenCalledWith(delegatorData.user_id)) and assert call order against orgUserService.switchToDelegator by comparing their spy invocation orders (e.g., expect(orgUserService.setDelegateeUserId.calls.first().invocationOrder).toBeLessThan(orgUserService.switchToDelegator.calls.first().invocationOrder)).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/core/services/org-user.service.ts`:
- Around line 145-149: The current operator chain calls switchMap inside a tap
callback so the delete('delegatee_id') promise is never executed or chained;
update the Rx pipeline so deletion is part of the observable chain — e.g., after
apiService.post<AuthResponse>('/orgusers/delegatee_refresh_token') use switchMap
to call authService.newRefreshToken(data.refresh_token) and then chain another
operator (switchMap/mergeMap + from(storageService.delete('delegatee_id')) or a
tap that returns a resolved observable) to perform and await
storageService.delete('delegatee_id') before emitting the refreshed auth object;
reference apiService.post, authService.newRefreshToken, storageService.delete,
and remove the unused switchMap inside tap so the cleanup runs reliably.
In `@src/app/fyle/view-expense/view-expense.page.ts`:
- Around line 396-415: The delegation ownership check (using
launchDarklyService.getVariation and delegationService.isDelegateeOwnedExpense)
is duplicated across view-expense, view-mileage, and view-per-diem and also
lacks error handling; extract this pattern into a shared Angular service (e.g.,
DelegationHelperService) that exposes a method like
checkDelegateeOwnership(expense$: Observable<{user_id:string}>, view:
ExpenseView) which performs the feature-flag check
(controlled_delegate_access_enabled), short-circuits when !enabled or view !==
ExpenseView.team, calls
delegationService.isDelegateeOwnedExpense(expense.user_id) when needed, and
includes catchError to default to false on errors, then have the pages subscribe
and call delegateeOwnedExpense.set(...) with the result instead of duplicating
the logic (refer to launchDarklyService.getVariation,
delegationService.isDelegateeOwnedExpense, delegateeOwnedExpense.set, and
ExpenseView.team).
In `@src/app/fyle/view-mileage/view-mileage.page.spec.ts`:
- Around line 134-143: Add unit tests to ViewMileagePage spec to cover
delegation scenarios: create tests that (1) set LaunchDarklyService.getVariation
to emit true and assert component.controlledDelegateAccessEnabled becomes true
after calling component.ionViewWillEnter() (use fakeAsync and tick), (2) set
DelegationService.isDelegateeOwnedExpense to resolveTo(true) and assert
component.delegateeOwnedExpense() returns true and the delete button is hidden,
and (3) simulate failures from LaunchDarklyService.getVariation and
DelegationService.isDelegateeOwnedExpense (make them throw/reject) and assert
the component handles errors gracefully (no uncaught exceptions and reasonable
default state). Use the existing jasmine spies (DelegationService,
LaunchDarklyService) wired in TestBed.configureTestingModule to modify
getVariation and isDelegateeOwnedExpense per test.
In `@src/app/fyle/view-mileage/view-mileage.page.ts`:
- Around line 352-371: The subscription chain in view-mileage.page.ts using
this.mileageExpense$ ->
switchMap(this.delegationService.isDelegateeOwnedExpense) lacks error handling,
so delegateeOwnedExpense stays null if the inner observable errors; update the
pipe in the mileageExpense$ branch inside the launchDarklyService.subscribe to
import and use catchError after switchMap to catch errors, set
this.delegateeOwnedExpense.set(false) (or a safe default) on error, and
optionally log the error, ensuring catchError is imported from 'rxjs/operators'
and that the change is applied around the switchMap(...) observable.
In `@src/app/fyle/view-per-diem/view-per-diem.page.spec.ts`:
- Around line 107-116: Add a test case in view-per-diem.page.spec.ts that
exercises the true-branch for delegate-access by overriding the spies: set
launchDarklyServiceSpy.getVariation to return of(true) for the
controlled_delegate_access_enabled flag and set
delegationServiceSpy.isDelegateeOwnedExpense to resolveTo(true); initialize the
component via TestBed (same setup used in the file) and assert the
restricted-action behavior (e.g., the restricted action method/component state
is blocked or the related button is disabled) so the restricted-action path is
explicitly covered and protected from regressions.
In `@src/app/fyle/view-per-diem/view-per-diem.page.ts`:
- Around line 259-278: The delegation check can reject and leave
delegateeOwnedExpense null without any observability; wrap the inner observable
chain (the switchMap/from(this.delegationService.isDelegateeOwnedExpense(...)))
with catchError (imported from rxjs/operators) so failures are caught, return
of(false) on error and call your logging mechanism (e.g., this.logger.error(...)
or console.error) to record the error, and ensure you still call
this.delegateeOwnedExpense.set(false) in the error path so the UI state is
deterministic; update the pipeline attached to perDiemExpense$ (and its
subscribe) to handle the caught value accordingly.
---
Outside diff comments:
In `@src/app/fyle/delegated-accounts/delegated-accounts.page.spec.ts`:
- Around line 123-139: The test stubs orgUserService.setDelegateeUserId but
never asserts it was invoked or that it ran before switchToDelegator; add
assertions after tick(500) to verify setDelegateeUserId was called once with
delegatorData.user_id
(expect(orgUserService.setDelegateeUserId).toHaveBeenCalledTimes(1);
expect(orgUserService.setDelegateeUserId).toHaveBeenCalledWith(delegatorData.user_id))
and assert call order against orgUserService.switchToDelegator by comparing
their spy invocation orders (e.g.,
expect(orgUserService.setDelegateeUserId.calls.first().invocationOrder).toBeLessThan(orgUserService.switchToDelegator.calls.first().invocationOrder)).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d79e4cbf-0111-4fb9-8216-77b1cbf8308c
📒 Files selected for processing (18)
src/app/core/services/auth.service.tssrc/app/core/services/delegation.service.spec.tssrc/app/core/services/delegation.service.tssrc/app/core/services/org-user.service.tssrc/app/fyle/delegated-accounts/delegated-accounts.page.spec.tssrc/app/fyle/delegated-accounts/delegated-accounts.page.tssrc/app/fyle/view-expense/view-expense.page.htmlsrc/app/fyle/view-expense/view-expense.page.spec.tssrc/app/fyle/view-expense/view-expense.page.tssrc/app/fyle/view-mileage/view-mileage.page.htmlsrc/app/fyle/view-mileage/view-mileage.page.spec.tssrc/app/fyle/view-mileage/view-mileage.page.tssrc/app/fyle/view-per-diem/view-per-diem.page.htmlsrc/app/fyle/view-per-diem/view-per-diem.page.spec.tssrc/app/fyle/view-per-diem/view-per-diem.page.tssrc/app/fyle/view-team-report/view-team-report.page.htmlsrc/app/fyle/view-team-report/view-team-report.page.spec.tssrc/app/fyle/view-team-report/view-team-report.page.ts
Comment on lines
+145
to
+149
| return this.apiService.post<AuthResponse>('/orgusers/delegatee_refresh_token').pipe( | ||
| switchMap((data) => this.authService.newRefreshToken(data.refresh_token)), | ||
| tap(() => { | ||
| switchMap((eou) => this.storageService.delete('delegatee_id').then(() => eou)); | ||
| }), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify no-op RxJS operator usage inside tap callback in this service.
rg -nP "tap\\s*\\(\\s*\\(.*\\)\\s*=>\\s*\\{[\\s\\S]*?\\bswitchMap\\s*\\(" src/app/core/services/org-user.service.ts -C 3Repository: fylein/fyle-mobile-app
Length of output: 48
🏁 Script executed:
# First, locate and inspect the org-user.service.ts file around the flagged lines
fd "org-user.service.ts" --type fRepository: fylein/fyle-mobile-app
Length of output: 107
🏁 Script executed:
# Read the org-user.service.ts file around lines 140-160 with line numbers
cat -n src/app/core/services/org-user.service.ts | sed -n '140,160p'Repository: fylein/fyle-mobile-app
Length of output: 1005
Uyir vasthundaa dhaa... switchMap moondhula tap-pul irukku, da! Code vandhuttu cleanup execute aagalae.
Lines 145–149: The switchMap inside the tap callback is just a ghost operator, rajah. When you create an operator without returning or piping it, it's as useful as a punching bag in an empty room. The delegatee_id deletion never executes — the cleanup code is lost in the observable void, and your delegate context stays like stale coffee.
The operator chain breaks because tap is a side-effect operator that doesn't care about what you do inside its callback. You're calling switchMap() but not chaining it — the promise to delete the key vanishes like morning mist.
🔥 Fix it like this, boss
-import { Observable, Subject } from 'rxjs';
+import { from, Observable, Subject } from 'rxjs';
...
switchToDelegatee(): Observable<ExtendedOrgUser> {
return this.apiService.post<AuthResponse>('/orgusers/delegatee_refresh_token').pipe(
switchMap((data) => this.authService.newRefreshToken(data.refresh_token)),
- tap(() => {
- switchMap((eou) => this.storageService.delete('delegatee_id').then(() => eou));
- }),
+ switchMap((eou) =>
+ from(this.storageService.delete('delegatee_id')).pipe(
+ map(() => eou),
+ ),
+ ),
);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/core/services/org-user.service.ts` around lines 145 - 149, The
current operator chain calls switchMap inside a tap callback so the
delete('delegatee_id') promise is never executed or chained; update the Rx
pipeline so deletion is part of the observable chain — e.g., after
apiService.post<AuthResponse>('/orgusers/delegatee_refresh_token') use switchMap
to call authService.newRefreshToken(data.refresh_token) and then chain another
operator (switchMap/mergeMap + from(storageService.delete('delegatee_id')) or a
tap that returns a resolved observable) to perform and await
storageService.delete('delegatee_id') before emitting the refreshed auth object;
reference apiService.post, authService.newRefreshToken, storageService.delete,
and remove the unused switchMap inside tap so the cleanup runs reliably.
Comment on lines
+396
to
+415
| this.launchDarklyService | ||
| .getVariation('controlled_delegate_access_enabled', false) | ||
| .pipe(take(1)) | ||
| .subscribe((enabled) => { | ||
| this.controlledDelegateAccessEnabled = enabled; | ||
|
|
||
| if (!enabled || this.view !== ExpenseView.team) { | ||
| this.delegateeOwnedExpense.set(false); | ||
| return; | ||
| } | ||
|
|
||
| this.expense$ | ||
| .pipe( | ||
| take(1), | ||
| switchMap((expense) => from(this.delegationService.isDelegateeOwnedExpense(expense.user_id))), | ||
| ) | ||
| .subscribe((isDelegateeOwnedExpense) => { | ||
| this.delegateeOwnedExpense.set(isDelegateeOwnedExpense); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Same delegation flow across three files - consider extracting to a shared service, boss!
This exact pattern is repeated in view-expense, view-mileage, and view-per-diem. Also missing error handling like the other pages.
🛡️ Add error handling (immediate fix)
this.expense$
.pipe(
take(1),
switchMap((expense) => from(this.delegationService.isDelegateeOwnedExpense(expense.user_id))),
+ catchError(() => of(false)),
)
.subscribe((isDelegateeOwnedExpense) => {
this.delegateeOwnedExpense.set(isDelegateeOwnedExpense);
});♻️ Consider extracting delegation check to a shared helper (future refactor)
The delegation ownership check logic is duplicated across three view pages. Consider creating a DelegationHelperService that encapsulates this pattern. Based on learnings, prefer creating an Angular service over static utility files for shared helper logic.
// delegation-helper.service.ts
`@Injectable`({ providedIn: 'root' })
export class DelegationHelperService {
private delegationService = inject(DelegationService);
private launchDarklyService = inject(LaunchDarklyService);
checkDelegateeOwnership(
expense$: Observable<{ user_id: string }>,
view: ExpenseView
): Observable<{ enabled: boolean; isOwned: boolean }> {
return this.launchDarklyService
.getVariation('controlled_delegate_access_enabled', false)
.pipe(
take(1),
switchMap((enabled) => {
if (!enabled || view !== ExpenseView.team) {
return of({ enabled: false, isOwned: false });
}
return expense$.pipe(
take(1),
switchMap((expense) =>
from(this.delegationService.isDelegateeOwnedExpense(expense.user_id))
),
map((isOwned) => ({ enabled: true, isOwned })),
catchError(() => of({ enabled: true, isOwned: 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.
Suggested change
| this.launchDarklyService | |
| .getVariation('controlled_delegate_access_enabled', false) | |
| .pipe(take(1)) | |
| .subscribe((enabled) => { | |
| this.controlledDelegateAccessEnabled = enabled; | |
| if (!enabled || this.view !== ExpenseView.team) { | |
| this.delegateeOwnedExpense.set(false); | |
| return; | |
| } | |
| this.expense$ | |
| .pipe( | |
| take(1), | |
| switchMap((expense) => from(this.delegationService.isDelegateeOwnedExpense(expense.user_id))), | |
| ) | |
| .subscribe((isDelegateeOwnedExpense) => { | |
| this.delegateeOwnedExpense.set(isDelegateeOwnedExpense); | |
| }); | |
| }); | |
| this.launchDarklyService | |
| .getVariation('controlled_delegate_access_enabled', false) | |
| .pipe(take(1)) | |
| .subscribe((enabled) => { | |
| this.controlledDelegateAccessEnabled = enabled; | |
| if (!enabled || this.view !== ExpenseView.team) { | |
| this.delegateeOwnedExpense.set(false); | |
| return; | |
| } | |
| this.expense$ | |
| .pipe( | |
| take(1), | |
| switchMap((expense) => from(this.delegationService.isDelegateeOwnedExpense(expense.user_id))), | |
| catchError(() => of(false)), | |
| ) | |
| .subscribe((isDelegateeOwnedExpense) => { | |
| this.delegateeOwnedExpense.set(isDelegateeOwnedExpense); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/fyle/view-expense/view-expense.page.ts` around lines 396 - 415, The
delegation ownership check (using launchDarklyService.getVariation and
delegationService.isDelegateeOwnedExpense) is duplicated across view-expense,
view-mileage, and view-per-diem and also lacks error handling; extract this
pattern into a shared Angular service (e.g., DelegationHelperService) that
exposes a method like checkDelegateeOwnership(expense$:
Observable<{user_id:string}>, view: ExpenseView) which performs the feature-flag
check (controlled_delegate_access_enabled), short-circuits when !enabled or view
!== ExpenseView.team, calls
delegationService.isDelegateeOwnedExpense(expense.user_id) when needed, and
includes catchError to default to false on errors, then have the pages subscribe
and call delegateeOwnedExpense.set(...) with the result instead of duplicating
the logic (refer to launchDarklyService.getVariation,
delegationService.isDelegateeOwnedExpense, delegateeOwnedExpense.set, and
ExpenseView.team).
Comment on lines
+134
to
+143
| const delegationServiceSpy = jasmine.createSpyObj('DelegationService', ['isDelegateeOwnedExpense']); | ||
| delegationServiceSpy.isDelegateeOwnedExpense.and.resolveTo(false); | ||
| const launchDarklyServiceSpy = jasmine.createSpyObj('LaunchDarklyService', ['getVariation']); | ||
| launchDarklyServiceSpy.getVariation.and.returnValue(of(false)); | ||
|
|
||
| TestBed.configureTestingModule({ | ||
| imports: [ViewMileagePage], | ||
| providers: [ | ||
| { provide: DelegationService, useValue: delegationServiceSpy }, | ||
| { provide: LaunchDarklyService, useValue: launchDarklyServiceSpy }, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
En vazhi thani vazhi! The mock setup is correct, but more test coverage needed, machaan!
The spies are wired correctly with default values. However, consider adding test cases for these scenarios to ensure full coverage of the delegation feature:
- When
controlled_delegate_access_enabledflag istrue - When
isDelegateeOwnedExpensereturnstrue(button should be hidden) - When LaunchDarkly/DelegationService calls fail (error handling)
🧪 Example test case for delegation-enabled scenario
it('should hide delete button when expense is delegatee-owned and flag is enabled', fakeAsync(() => {
const delegationServiceSpy = TestBed.inject(DelegationService) as jasmine.SpyObj<DelegationService>;
const launchDarklyServiceSpy = TestBed.inject(LaunchDarklyService) as jasmine.SpyObj<LaunchDarklyService>;
launchDarklyServiceSpy.getVariation.and.returnValue(of(true));
delegationServiceSpy.isDelegateeOwnedExpense.and.resolveTo(true);
activateRouteMock.snapshot.params.view = ExpenseView.team;
component.ionViewWillEnter();
tick(500);
expect(component.controlledDelegateAccessEnabled).toBeTrue();
expect(component.delegateeOwnedExpense()).toBeTrue();
}));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/fyle/view-mileage/view-mileage.page.spec.ts` around lines 134 - 143,
Add unit tests to ViewMileagePage spec to cover delegation scenarios: create
tests that (1) set LaunchDarklyService.getVariation to emit true and assert
component.controlledDelegateAccessEnabled becomes true after calling
component.ionViewWillEnter() (use fakeAsync and tick), (2) set
DelegationService.isDelegateeOwnedExpense to resolveTo(true) and assert
component.delegateeOwnedExpense() returns true and the delete button is hidden,
and (3) simulate failures from LaunchDarklyService.getVariation and
DelegationService.isDelegateeOwnedExpense (make them throw/reject) and assert
the component handles errors gracefully (no uncaught exceptions and reasonable
default state). Use the existing jasmine spies (DelegationService,
LaunchDarklyService) wired in TestBed.configureTestingModule to modify
getVariation and isDelegateeOwnedExpense per test.
Comment on lines
+352
to
+371
| this.launchDarklyService | ||
| .getVariation('controlled_delegate_access_enabled', false) | ||
| .pipe(take(1)) | ||
| .subscribe((enabled) => { | ||
| this.controlledDelegateAccessEnabled = enabled; | ||
|
|
||
| if (!enabled || this.view !== ExpenseView.team) { | ||
| this.delegateeOwnedExpense.set(false); | ||
| return; | ||
| } | ||
|
|
||
| this.mileageExpense$ | ||
| .pipe( | ||
| take(1), | ||
| switchMap((expense) => from(this.delegationService.isDelegateeOwnedExpense(expense.user_id))), | ||
| ) | ||
| .subscribe((isDelegateeOwnedExpense) => { | ||
| this.delegateeOwnedExpense.set(isDelegateeOwnedExpense); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Same delegation flow, same concern - add error handling, thalaiva!
Identical pattern to view-per-diem - if isDelegateeOwnedExpense fails, the signal stays null. Add error handling for consistent behavior across all view pages.
🛡️ Proposed fix to add error handling
this.mileageExpense$
.pipe(
take(1),
switchMap((expense) => from(this.delegationService.isDelegateeOwnedExpense(expense.user_id))),
+ catchError(() => of(false)),
)
.subscribe((isDelegateeOwnedExpense) => {
this.delegateeOwnedExpense.set(isDelegateeOwnedExpense);
});Add catchError to imports from rxjs/operators.
📝 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.
Suggested change
| this.launchDarklyService | |
| .getVariation('controlled_delegate_access_enabled', false) | |
| .pipe(take(1)) | |
| .subscribe((enabled) => { | |
| this.controlledDelegateAccessEnabled = enabled; | |
| if (!enabled || this.view !== ExpenseView.team) { | |
| this.delegateeOwnedExpense.set(false); | |
| return; | |
| } | |
| this.mileageExpense$ | |
| .pipe( | |
| take(1), | |
| switchMap((expense) => from(this.delegationService.isDelegateeOwnedExpense(expense.user_id))), | |
| ) | |
| .subscribe((isDelegateeOwnedExpense) => { | |
| this.delegateeOwnedExpense.set(isDelegateeOwnedExpense); | |
| }); | |
| }); | |
| this.launchDarklyService | |
| .getVariation('controlled_delegate_access_enabled', false) | |
| .pipe(take(1)) | |
| .subscribe((enabled) => { | |
| this.controlledDelegateAccessEnabled = enabled; | |
| if (!enabled || this.view !== ExpenseView.team) { | |
| this.delegateeOwnedExpense.set(false); | |
| return; | |
| } | |
| this.mileageExpense$ | |
| .pipe( | |
| take(1), | |
| switchMap((expense) => from(this.delegationService.isDelegateeOwnedExpense(expense.user_id))), | |
| catchError(() => of(false)), | |
| ) | |
| .subscribe((isDelegateeOwnedExpense) => { | |
| this.delegateeOwnedExpense.set(isDelegateeOwnedExpense); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/fyle/view-mileage/view-mileage.page.ts` around lines 352 - 371, The
subscription chain in view-mileage.page.ts using this.mileageExpense$ ->
switchMap(this.delegationService.isDelegateeOwnedExpense) lacks error handling,
so delegateeOwnedExpense stays null if the inner observable errors; update the
pipe in the mileageExpense$ branch inside the launchDarklyService.subscribe to
import and use catchError after switchMap to catch errors, set
this.delegateeOwnedExpense.set(false) (or a safe default) on error, and
optionally log the error, ensuring catchError is imported from 'rxjs/operators'
and that the change is applied around the switchMap(...) observable.
Comment on lines
+107
to
+116
| const delegationServiceSpy = jasmine.createSpyObj('DelegationService', ['isDelegateeOwnedExpense']); | ||
| delegationServiceSpy.isDelegateeOwnedExpense.and.resolveTo(false); | ||
| const launchDarklyServiceSpy = jasmine.createSpyObj('LaunchDarklyService', ['getVariation']); | ||
| launchDarklyServiceSpy.getVariation.and.returnValue(of(false)); | ||
|
|
||
| TestBed.configureTestingModule({ | ||
| imports: [ViewPerDiemPage], | ||
| providers: [ | ||
| { provide: DelegationService, useValue: delegationServiceSpy }, | ||
| { provide: LaunchDarklyService, useValue: launchDarklyServiceSpy }, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add one true-branch test for new delegate-access wiring.
Nice setup, but all tests now run with controlled_delegate_access_enabled = false and isDelegateeOwnedExpense = false. Add one test overriding these to true so the restricted-action path is explicitly protected from regressions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/fyle/view-per-diem/view-per-diem.page.spec.ts` around lines 107 -
116, Add a test case in view-per-diem.page.spec.ts that exercises the
true-branch for delegate-access by overriding the spies: set
launchDarklyServiceSpy.getVariation to return of(true) for the
controlled_delegate_access_enabled flag and set
delegationServiceSpy.isDelegateeOwnedExpense to resolveTo(true); initialize the
component via TestBed (same setup used in the file) and assert the
restricted-action behavior (e.g., the restricted action method/component state
is blocked or the related button is disabled) so the restricted-action path is
explicitly covered and protected from regressions.
Comment on lines
+259
to
+278
| this.launchDarklyService | ||
| .getVariation('controlled_delegate_access_enabled', false) | ||
| .pipe(take(1)) | ||
| .subscribe((enabled) => { | ||
| this.controlledDelegateAccessEnabled = enabled; | ||
|
|
||
| if (!enabled || this.view !== ExpenseView.team) { | ||
| this.delegateeOwnedExpense.set(false); | ||
| return; | ||
| } | ||
|
|
||
| this.perDiemExpense$ | ||
| .pipe( | ||
| take(1), | ||
| switchMap((expense) => from(this.delegationService.isDelegateeOwnedExpense(expense.user_id))), | ||
| ) | ||
| .subscribe((isDelegateeOwnedExpense) => { | ||
| this.delegateeOwnedExpense.set(isDelegateeOwnedExpense); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
No error handling in delegation flow, boss - what if it fails?
The nested subscription will auto-complete thanks to take(1), but if isDelegateeOwnedExpense rejects, delegateeOwnedExpense stays null forever with no feedback. While the template handles null safely (button stays hidden), consider adding error handling for observability.
🛡️ Proposed fix to add error handling
this.perDiemExpense$
.pipe(
take(1),
switchMap((expense) => from(this.delegationService.isDelegateeOwnedExpense(expense.user_id))),
+ catchError(() => of(false)),
)
.subscribe((isDelegateeOwnedExpense) => {
this.delegateeOwnedExpense.set(isDelegateeOwnedExpense);
});Add catchError to imports from rxjs/operators.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/fyle/view-per-diem/view-per-diem.page.ts` around lines 259 - 278, The
delegation check can reject and leave delegateeOwnedExpense null without any
observability; wrap the inner observable chain (the
switchMap/from(this.delegationService.isDelegateeOwnedExpense(...))) with
catchError (imported from rxjs/operators) so failures are caught, return
of(false) on error and call your logging mechanism (e.g., this.logger.error(...)
or console.error) to record the error, and ensure you still call
this.delegateeOwnedExpense.set(false) in the error path so the UI state is
deterministic; update the pipeline attached to perDiemExpense$ (and its
subscribe) to handle the caught value accordingly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Clickup
Please add link here
app.clickup.com
Code Coverage
Please add code coverage here
UI Preview
Please add screenshots for UI changes
Summary by CodeRabbit
Release Notes
New Features
Improvements
Tests