fix: Uses guard instead of redirection to reach onboarding page when opening the mobile app #3438
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughIn a thunderous display of code prowess, this pull request transforms the application's onboarding and navigation logic! A new Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✨ 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: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
src/app/app-routing.module.ts(1 hunks)src/app/auth/switch-org/switch-org.page.ts(1 hunks)src/app/core/guards/onboarding.guard.ts(1 hunks)src/app/core/services/spender-onboarding.service.ts(2 hunks)src/app/fyle/fyle-routing.module.ts(1 hunks)src/app/fyle/spender-onboarding/spender-onboarding.page.ts(0 hunks)src/app/post-verification/invited-user/invited-user.page.ts(2 hunks)
💤 Files with no reviewable changes (1)
- src/app/fyle/spender-onboarding/spender-onboarding.page.ts
🧰 Additional context used
🪛 eslint
src/app/app-routing.module.ts
[error] 6-6: 'OnboardingGuard' is defined but never used.
(@typescript-eslint/no-unused-vars)
src/app/core/guards/onboarding.guard.ts
[error] 14-14: 'next' is defined but never used.
(@typescript-eslint/no-unused-vars)
[error] 15-15: 'state' is defined but never used.
(@typescript-eslint/no-unused-vars)
🪛 GitHub Check: Run linters
src/app/app-routing.module.ts
[failure] 6-6:
'OnboardingGuard' is defined but never used
src/app/core/guards/onboarding.guard.ts
[failure] 14-14:
'next' is defined but never used
[failure] 15-15:
'state' is defined but never used
🪛 GitHub Actions: Lint
src/app/app-routing.module.ts
[error] 6-6: 'OnboardingGuard' is defined but never used
🔇 Additional comments (2)
src/app/fyle/fyle-routing.module.ts (1)
5-5: Mind it! The routing configuration looks solid!The OnboardingGuard is properly integrated with the my_dashboard route while maintaining the existing OptInGuard functionality. This is how we do it in style!
Also applies to: 11-11
src/app/auth/switch-org/switch-org.page.ts (1)
321-339: Let's verify our navigation style is consistent everywhere!Check if all components use the new navigation pattern consistently.
✅ Verification successful
Mind it! Your navigation style is perfectly consistent, just like my style in movies!
The navigation pattern
['/', 'enterprise', ...]is followed religiously throughout the codebase, like a well-choreographed action sequence. Your code in switch-org.page.ts matches this pattern perfectly, making it a blockbuster implementation!🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all files that might contain navigation logic to ensure consistent implementation # Look for potential navigation to dashboard or onboarding rg -l "navigate.*(?:dashboard|onboarding)" --type ts # Look for old navigation patterns that might need updating rg -l "forkJoin.*orgSettingsService.*getOnboardingStatus" --type tsLength of output: 2265
Script:
#!/bin/bash # Search for enterprise route navigation patterns rg "navigate.*enterprise" -A 2 -B 2 --type ts # Also check for any router imports to ensure we're using the same navigation method ast-grep --pattern 'import { $$ } from "@angular/router"'Length of output: 66887
| private checkCCCEnabled(orgSettings: OrgSettings): boolean { | ||
| return !!orgSettings.corporate_credit_card_settings.allowed && !!orgSettings.corporate_credit_card_settings.enabled; | ||
| } |
There was a problem hiding this comment.
Careful! Null checks needed to prevent runtime errors.
If orgSettings.corporate_credit_card_settings is undefined, accessing its properties will cause a crash. Let's ensure we check for its existence before using it.
Apply this diff to add the null check:
private checkCCCEnabled(orgSettings: OrgSettings): boolean {
- return !!orgSettings.corporate_credit_card_settings.allowed && !!orgSettings.corporate_credit_card_settings.enabled;
+ return (
+ !!orgSettings.corporate_credit_card_settings &&
+ !!orgSettings.corporate_credit_card_settings.allowed &&
+ !!orgSettings.corporate_credit_card_settings.enabled
+ );
}📝 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.
| private checkCCCEnabled(orgSettings: OrgSettings): boolean { | |
| return !!orgSettings.corporate_credit_card_settings.allowed && !!orgSettings.corporate_credit_card_settings.enabled; | |
| } | |
| private checkCCCEnabled(orgSettings: OrgSettings): boolean { | |
| return ( | |
| !!orgSettings.corporate_credit_card_settings && | |
| !!orgSettings.corporate_credit_card_settings.allowed && | |
| !!orgSettings.corporate_credit_card_settings.enabled | |
| ); | |
| } |
| next: ActivatedRouteSnapshot, | ||
| state: RouterStateSnapshot |
There was a problem hiding this comment.
Unused parameters in canActivate method.
The parameters next and state are defined but not used, which causes linting errors.
You can fix this by prefixing the parameters with an underscore or removing them if they're not needed:
canActivate(
- next: ActivatedRouteSnapshot,
- state: RouterStateSnapshot
+ _next: ActivatedRouteSnapshot,
+ _state: RouterStateSnapshot
): Observable<boolean | UrlTree> | Promise<boolean | UrlTree> | boolean | UrlTree {📝 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.
| next: ActivatedRouteSnapshot, | |
| state: RouterStateSnapshot | |
| _next: ActivatedRouteSnapshot, | |
| _state: RouterStateSnapshot |
🧰 Tools
🪛 eslint
[error] 14-14: 'next' is defined but never used.
(@typescript-eslint/no-unused-vars)
[error] 15-15: 'state' is defined but never used.
(@typescript-eslint/no-unused-vars)
🪛 GitHub Check: Run linters
[failure] 14-14:
'next' is defined but never used
[failure] 15-15:
'state' is defined but never used
src/app/app-routing.module.ts
Outdated
| @@ -3,11 +3,12 @@ | |||
| import { PreloadAllModules, RouterModule, Routes } from '@angular/router'; | |||
| import { AuthGuard } from './core/guards/auth.guard'; | |||
| import { VerifiedOrgAuthGuard } from './core/guards/verified-org-auth.guard'; | |||
| import { OnboardingGuard } from './core/guards/onboarding.guard'; | |||
There was a problem hiding this comment.
'OnboardingGuard' is imported but not used.
The OnboardingGuard is imported but not utilized in this file, leading to a linting error.
Either remove the unused import:
-import { OnboardingGuard } from './core/guards/onboarding.guard';Or, if you intend to use it, add it to the appropriate route:
{
path: '',
- redirectTo: 'enterprise/my_dashboard',
+ redirectTo: 'enterprise/my_dashboard',
+ canActivate: [OnboardingGuard],
pathMatch: 'full',
},Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 eslint
[error] 6-6: 'OnboardingGuard' is defined but never used.
(@typescript-eslint/no-unused-vars)
🪛 GitHub Check: Run linters
[failure] 6-6:
'OnboardingGuard' is defined but never used
🪛 GitHub Actions: Lint
[error] 6-6: 'OnboardingGuard' is defined but never used
| this.spenderOnboardingService | ||
| .checkForRedirectionToOnboarding() | ||
| .pipe( | ||
| map((shouldProceedToOnboarding) => { | ||
| if (shouldProceedToOnboarding) { | ||
| this.router.navigate(['/', 'enterprise', 'spender_onboarding']); | ||
| } else { | ||
| this.router.navigate(['/', 'enterprise', 'my_dashboard']); | ||
| } | ||
| }) | ||
| ) | ||
| .subscribe(); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
What a transformation! But let's add some style to error handling, macha!
The simplified navigation logic using checkForRedirectionToOnboarding is clean and effective. However, we should handle potential errors with the same style we handle success!
Add error handling like this, superstar:
this.spenderOnboardingService
.checkForRedirectionToOnboarding()
.pipe(
map((shouldProceedToOnboarding) => {
if (shouldProceedToOnboarding) {
this.router.navigate(['/', 'enterprise', 'spender_onboarding']);
} else {
this.router.navigate(['/', 'enterprise', 'my_dashboard']);
}
}),
+ catchError((error) => {
+ console.error('Navigation failed:', error);
+ return this.router.navigate(['/', 'auth', 'login']);
+ })
)
.subscribe();📝 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.
| this.spenderOnboardingService | |
| .checkForRedirectionToOnboarding() | |
| .pipe( | |
| map((shouldProceedToOnboarding) => { | |
| if (shouldProceedToOnboarding) { | |
| this.router.navigate(['/', 'enterprise', 'spender_onboarding']); | |
| } else { | |
| this.router.navigate(['/', 'enterprise', 'my_dashboard']); | |
| } | |
| }) | |
| ) | |
| .subscribe(); | |
| this.spenderOnboardingService | |
| .checkForRedirectionToOnboarding() | |
| .pipe( | |
| map((shouldProceedToOnboarding) => { | |
| if (shouldProceedToOnboarding) { | |
| this.router.navigate(['/', 'enterprise', 'spender_onboarding']); | |
| } else { | |
| this.router.navigate(['/', 'enterprise', 'my_dashboard']); | |
| } | |
| }), | |
| catchError((error) => { | |
| console.error('Navigation failed:', error); | |
| return this.router.navigate(['/', 'auth', 'login']); | |
| }) | |
| ) | |
| .subscribe(); |
| this.spenderOnboardingService | ||
| .checkForRedirectionToOnboarding() | ||
| .pipe( | ||
| map((shouldProceedToOnboarding) => { | ||
| if (shouldProceedToOnboarding) { | ||
| this.router.navigate(['/', 'enterprise', 'spender_onboarding']); | ||
| } else { | ||
| this.router.navigate([ | ||
| '/', | ||
| 'enterprise', | ||
| 'my_dashboard', | ||
| { | ||
| openSMSOptInDialog: openOptInDialog, | ||
| }, | ||
| ]); | ||
| } | ||
| }) | ||
| ) | ||
| .subscribe(); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Mind-blowing implementation, but let's add some punch with tests!
The navigation logic is perfectly aligned with the invited-user page implementation, and smoothly handles the SMS opt-in dialog. But every superstar needs a good backup!
Would you like me to generate unit tests for this navigation logic? We'll test scenarios like:
- Redirection to onboarding
- Redirection to dashboard with SMS opt-in
- Error handling
| } | ||
| return true; | ||
| }) | ||
| ); |
There was a problem hiding this comment.
can we redirect to dashboard if the API throws some error?
There was a problem hiding this comment.
will add this in a follow up
|
|
||
| const routes: Routes = [ | ||
| { | ||
| path: 'my_dashboard', | ||
| loadChildren: () => import('./dashboard/dashboard.module').then((m) => m.DashboardPageModule), | ||
| canActivate: [OnboardingGuard], |
There was a problem hiding this comment.
might have to check if this really solves the white screen that appears for few seconds in a low network.
If it is not solved, can we land the user to the dashboard and then check if user has to go to spender onboarding flow and redirect?
There was a problem hiding this comment.
Going to the dashboard will still be jarring UI in this case, what I can do is try adding a loader with the guard over the white screen (same as before). The transition won't be as obvious as before though.
There was a problem hiding this comment.
or I can add a localstorage key to determine if onboarding is complete, even before checking the API calls
There was a problem hiding this comment.
Will add this in a follow up
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/app/core/services/spender-onboarding.service.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build (12.x)
- GitHub Check: gitStream.cm
🔇 Additional comments (3)
src/app/core/services/spender-onboarding.service.ts (3)
125-126: Mind the null, avoid the fall!Boss, if
orgSettings.corporate_credit_card_settingsis undefined, accessing its properties will crash the app faster than a speeding bullet! Let's add a check to ensure it exists before we use it.Apply this diff to add the null check:
private checkCCCEnabled(orgSettings: OrgSettings): boolean { + return ( + !!orgSettings.corporate_credit_card_settings && !!orgSettings.corporate_credit_card_settings.allowed && !!orgSettings.corporate_credit_card_settings.enabled + ); }
145-150: Currency check sharper than my style, but let's think global!We're checking if
currency === 'USD'to proceed. Are we sure we want to limit this to USD only? Let's ensure we're not leaving out other currencies if that's not the intention.
109-110:⚠️ Potential issueDouble power when single is enough!
We're using
isCCCEnabled && isCardFeedEnabledto determineareCardsEnabled. Shouldn't we proceed if either one is enabled? Using||might be the way.Apply this diff if that's the case:
const shouldProceedToOnboarding = this.shouldProceedToOnboarding( eou.org.currency, restrictedOrgs, - isCCCEnabled && isCardFeedEnabled, + isCCCEnabled || isCardFeedEnabled, onboardingStatus );Likely invalid or redundant comment.
| } | ||
|
|
||
| private isRestrictedOrg(orgSettings: OrgSettings, isUserFromINCluster: boolean): boolean { | ||
| return orgSettings.org_id === 'orgp5onHZThs' || isUserFromINCluster; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Hardcoding org IDs? Let's keep it flexible like me!
Thalaiva, using a hardcoded org_id like 'orgp5onHZThs' is risky business. If that ID changes, our code breaks. Consider fetching this ID from a configuration or making it dynamic.
| this.orgSettingsService.get(), | ||
| this.getOnboardingStatus(), | ||
| from(this.utilityService.isUserFromINCluster()), | ||
| this.authService.getEou(), | ||
| ]).pipe( |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Too many cooks spoil the broth, let's simplify!
Superstar, we're fetching multiple observables with forkJoin. Let's make sure all these are necessary and optimize our calls. Unnecessary calls can slow us down.
bistaastha
merged commit 3516b3f
into
FYLE-86cx2t82k-base-feature-branch
This PR includes:
Summary by CodeRabbit
Release Notes
New Features
Improvements
Changes