feat: Expose QBD Direct Integration #1052
Conversation
WalkthroughThe changes in this pull request involve modifications to several files to introduce a new environment variable Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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: 5
🧹 Outside diff range and nitpick comments (5)
src/app/core/models/branding/expose-app-configuration.model.ts (1)
13-13: Add JSDoc comment to document the QBD_DIRECT flag.
The type addition follows the existing pattern and maintains consistency. However, please add documentation to explain the purpose and impact of this configuration flag.
+ /** Flag to enable/disable QBD Direct integration features */
QBD_DIRECT: boolean;.github/workflows/unit-test.yml (1)
35-35: Consider documenting the new environment variable.
Since this is part of a new QBD Direct Integration feature, it would be helpful to add a comment explaining the purpose and usage of this environment variable, especially since the PR description is empty.
Add a comment above the environment variable:
+ # API URL for QBD Direct Integration service used during testing
QBD_DIRECT_API_URL: "http://lolo.fyle.tech"src/app/branding/expose-app-config.ts (1)
14-15: Add documentation for the QBD_DIRECT feature flag.
The new QBD_DIRECT configuration has been added across all environments, but its purpose and implications are not documented. Consider adding a comment explaining:
- The purpose of this feature flag
- The criteria for enabling/disabling it in different environments
- Any dependencies or related configurations needed
Also applies to: 26-27, 38-39, 52-53, 64-65
scripts/setup_env.js (1)
Line range hint 1-45: Consider adding JSDoc comments to document the environment configuration.
To improve maintainability and make it easier for other developers to understand the purpose of each configuration, consider adding JSDoc comments to document the environment object and its properties.
Example addition:
+/**
+ * Environment configuration for the application.
+ * @typedef {Object} Environment
+ * @property {string} production - Production mode flag
+ * @property {string} fyle_client_id - Fyle OAuth client ID
+ * @property {string} qbd_direct_api_url - QBD Direct API endpoint URL
+ * ... (document other properties)
+ */
const environment = {src/app/core/services/common/helper.service.ts (1)
55-55: LGTM! Consider adding type definitions for environment variables.
The implementation follows the consistent pattern used for other integrations. However, to prevent potential runtime issues, consider adding type definitions for the environment variables.
Add this interface to your environment.ts files:
interface Environment {
production: boolean;
cluster_domain_api_url: string;
qbd_direct_api_url: string;
// ... other environment variables
}📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- .github/workflows/unit-test.yml (1 hunks)
- run.sh (1 hunks)
- scripts/setup_env.js (1 hunks)
- src/app/auth/login/login.component.ts (3 hunks)
- src/app/branding/expose-app-config.ts (5 hunks)
- src/app/core/models/branding/expose-app-configuration.model.ts (1 hunks)
- src/app/core/services/common/helper.service.ts (1 hunks)
- src/app/core/services/qbd-direct/qbd-direct-core/qbd-direct-auth.service.ts (1 hunks)
🧰 Additional context used
🪛 Shellcheck
run.sh
[warning] 13-13: Quotes/backslashes in this variable will not be respected.
(SC2090)
🔇 Additional comments (10)
src/app/core/models/branding/expose-app-configuration.model.ts (1)
13-13: Verify QBD_DIRECT configuration across environments.
Let's ensure this flag is properly configured across all environments and branding configurations.
✅ Verification successful
Let me check the actual configuration values to ensure consistency.
QBD_DIRECT configuration is properly set across environments and branding configs
The QBD_DIRECT flag is consistently configured across:
- Environment-specific settings in
expose-app-config.tswith appropriate boolean values:- Enabled for some environments (
true) - Disabled for production environments (
false)
- Enabled for some environments (
- Help documentation links in both Fyle and C1 branding configs
- Video tutorial links in both branding configurations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for QBD_DIRECT configuration in environment files and constants
# Test 1: Search for QBD_DIRECT configuration in environment files
echo "Checking environment configurations..."
rg -l "QBD_DIRECT" src/environments/
# Test 2: Search for QBD_DIRECT in configuration constants
echo "Checking configuration constants..."
rg -l "QBD_DIRECT" src/app/branding/
Length of output: 359
Script:
#!/bin/bash
# Check the actual QBD_DIRECT configuration values
echo "Content of branding config files:"
echo -e "\nfyle-branding-config.ts:"
rg -A 2 -B 2 "QBD_DIRECT" src/app/branding/fyle-branding-config.ts
echo -e "\nc1-branding-config.ts:"
rg -A 2 -B 2 "QBD_DIRECT" src/app/branding/c1-branding-config.ts
echo -e "\nexpose-app-config.ts:"
rg -A 2 -B 2 "QBD_DIRECT" src/app/branding/expose-app-config.ts
Length of output: 3190
src/app/core/services/qbd-direct/qbd-direct-core/qbd-direct-auth.service.ts (2)
2-4: LGTM! All required imports are present.
The imports are properly structured and follow Angular conventions.
11-13: LGTM! Constructor follows Angular DI pattern.
The ApiService dependency is properly injected using the private access modifier.
src/app/branding/expose-app-config.ts (3)
26-27: Confirm regional deployment strategy.
The feature is disabled in both India (production-1-in) and US (production-1-us) production environments. Please confirm if this is intentional and if there's a planned timeline for enabling it in production.
Also applies to: 38-39
Line range hint 1-3: Update TypeScript interface to include QBD_DIRECT property.
The AllowedAppsConfiguration interface should be updated to include the new QBD_DIRECT property to ensure type safety.
Let's verify if the interface is properly updated:
14-15: Verify the staging-only enablement strategy.
The QBD_DIRECT feature is only enabled in the staging environment. This suggests a controlled rollout, but we should verify if this aligns with the intended deployment strategy.
Let's check for any related configuration or documentation:
run.sh (1)
13-13: LGTM! The QBD Direct API URL substitution is properly implemented.
The new substitution follows the established pattern and integrates well with the existing environment variable replacement logic.
Let's verify that the QBD_DIRECT_API_URL environment variable is properly configured across the application:
✅ Verification successful
Environment variable QBD_DIRECT_API_URL is properly configured and used
The verification confirms:
- The placeholder
{{QBD_DIRECT_API_URL}}is correctly used insetup_env.jswith proper fallback handling - The environment variable is configured in the CI workflow (
.github/workflows/unit-test.yml) - The substitution in
run.shfollows the established pattern for replacing the placeholder
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify QBD_DIRECT_API_URL configuration
# Test: Check for placeholder usage in HTML/TS/JS files
rg -g '*.{html,ts,js}' "{{QBD_DIRECT_API_URL}}"
# Test: Check for environment variable configuration in deployment files
rg "QBD_DIRECT_API_URL" .github/
Length of output: 265
Script:
#!/bin/bash
# Fix the regex pattern and search for QBD_DIRECT_API_URL placeholder
rg -F "{{QBD_DIRECT_API_URL}}" -g "*.{html,ts,js}"
# Check environment variable configuration in other deployment-related files
fd -e yaml -e yml -e env | xargs rg "QBD_DIRECT_API_URL"
# Check for any direct usage of QBD_DIRECT_API_URL in the codebase
rg "QBD_DIRECT_API_URL" -g "!{*.md,*.sh}"
Length of output: 431
🧰 Tools
🪛 Shellcheck
[warning] 13-13: Quotes/backslashes in this variable will not be respected.
(SC2090)
scripts/setup_env.js (1)
10-10: LGTM! The new QBD Direct API URL configuration follows the established patterns.
The addition of qbd_direct_api_url is consistent with:
- The existing environment variable handling pattern
- The naming conventions used throughout the file
- The placement near other QBD-related configurations
src/app/auth/login/login.component.ts (1)
20-20: LGTM: Import and dependency injection follow Angular best practices.
The QbdDirectAuthService is properly imported and injected following the established pattern in the component.
Also applies to: 41-41
src/app/core/services/common/helper.service.ts (1)
55-55: Verify environment variable setup.
Ensure that QBD_DIRECT_API_URL is properly configured across all deployment environments.
| import { Injectable } from '@angular/core'; | ||
| import { Observable } from 'rxjs'; | ||
| import { Token } from 'src/app/core/models/misc/token.model'; | ||
| import { ApiService } from '../../common/api.service'; | ||
|
|
||
| @Injectable({ | ||
| providedIn: 'root' | ||
| }) | ||
| export class QbdDirectAuthService { | ||
|
|
||
| constructor() { } | ||
| constructor( | ||
| private apiService: ApiService | ||
| ) { } | ||
|
|
||
| login(refresh_token: string): Observable<Token> { | ||
| return this.apiService.post('/auth/login_with_refresh_token/', { refresh_token: refresh_token }); | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Method name inconsistency detected in QbdDirectAuthService
The implementation differs from other auth services in the codebase:
- All other auth services use
loginWithRefreshTokenas the method name - Only QbdDirectAuthService uses
loginfor the same functionality - The endpoint and parameters are consistent across all services
🔗 Analysis chain
Verify consistency with other authentication services.
Let's ensure this implementation follows the same patterns as other authentication services in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar authentication service implementations
echo "Searching for other auth services..."
rg -t ts "login.*refresh_token" --glob "**/*auth.service.ts"
echo "Checking error handling patterns..."
ast-grep --pattern 'catchError((error: HttpErrorResponse) => { $$$ })'
Length of output: 2518
| login(refresh_token: string): Observable<Token> { | ||
| return this.apiService.post('/auth/login_with_refresh_token/', { refresh_token: refresh_token }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing error handling and type safety.
While the implementation is functional, consider these improvements for better robustness:
- Add input validation for refresh_token
- Handle API errors explicitly
- Use a typed interface for the request payload
- Consider moving the API endpoint to a constants file
Here's a suggested implementation:
+ interface LoginRequest {
+ refresh_token: string;
+ }
+
+ private readonly AUTH_ENDPOINT = '/auth/login_with_refresh_token/';
+
login(refresh_token: string): Observable<Token> {
- return this.apiService.post('/auth/login_with_refresh_token/', { refresh_token: refresh_token });
+ if (!refresh_token?.trim()) {
+ throw new Error('Refresh token is required');
+ }
+
+ const payload: LoginRequest = { refresh_token };
+
+ return this.apiService.post<Token>(this.AUTH_ENDPOINT, payload).pipe(
+ catchError((error: HttpErrorResponse) => {
+ console.error('QBD Direct authentication failed:', error);
+ throw error;
+ })
+ );
}📝 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.
| login(refresh_token: string): Observable<Token> { | |
| return this.apiService.post('/auth/login_with_refresh_token/', { refresh_token: refresh_token }); | |
| } | |
| interface LoginRequest { | |
| refresh_token: string; | |
| } | |
| private readonly AUTH_ENDPOINT = '/auth/login_with_refresh_token/'; | |
| login(refresh_token: string): Observable<Token> { | |
| if (!refresh_token?.trim()) { | |
| throw new Error('Refresh token is required'); | |
| } | |
| const payload: LoginRequest = { refresh_token }; | |
| return this.apiService.post<Token>(this.AUTH_ENDPOINT, payload).pipe( | |
| catchError((error: HttpErrorResponse) => { | |
| console.error('QBD Direct authentication failed:', error); | |
| throw error; | |
| }) | |
| ); | |
| } |
| FYLE_APP_URL: "http://lolo.fyle.tech" | ||
| SI_API_URL: "http://lolo.fyle.tech" | ||
| SAGE300_API_URL: "http://lolo.fyle.tech" | ||
| QBD_DIRECT_API_URL": "http://lolo.fyle.tech" |
There was a problem hiding this comment.
Fix syntax error in environment variable declaration.
There's an extra quote in the environment variable key name which will cause the workflow to fail.
Apply this fix:
- QBD_DIRECT_API_URL": "http://lolo.fyle.tech"
+ QBD_DIRECT_API_URL: "http://lolo.fyle.tech"📝 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.
| QBD_DIRECT_API_URL": "http://lolo.fyle.tech" | |
| QBD_DIRECT_API_URL: "http://lolo.fyle.tech" |
| if (this.exposeApps?.QBD_DIRECT) { | ||
| this.helperService.setBaseApiURL(AppUrl.QBD_DIRECT); | ||
| this.qbdDirectAuthService.login(clusterDomainWithToken.tokens.refresh_token).subscribe(); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and manage subscription.
While the authentication logic follows the established pattern, consider these improvements:
- Add error handling for the login call
- Manage the subscription to prevent memory leaks
if (this.exposeApps?.QBD_DIRECT) {
this.helperService.setBaseApiURL(AppUrl.QBD_DIRECT);
- this.qbdDirectAuthService.login(clusterDomainWithToken.tokens.refresh_token).subscribe();
+ this.qbdDirectAuthService.login(clusterDomainWithToken.tokens.refresh_token).subscribe({
+ error: (error) => {
+ console.error('QBD Direct login failed:', error);
+ // Consider showing user-friendly error message
+ }
+ });
}Consider implementing one of these approaches to manage subscriptions:
- Store subscription in a class property and unsubscribe in ngOnDestroy
- Use the async pipe in the template
- Use takeUntil with a destroy$ subject
Committable suggestion was skipped due to low confidence.
💡 Codebase verification
Test coverage needs improvement and architectural refactoring is recommended
The verification reveals:
- All auth-related test files are marked with
xdescribe(disabled) - QbdDirectAuthService has only a basic creation test
- LoginComponent's test coverage is minimal and doesn't cover integration paths
- Current implementation shows significant code duplication in handling different integrations
Specific findings:
- The login component has grown complex with 8+ auth service dependencies
- Similar login pattern repeated for each integration (QBD, QBD_DIRECT, SAGE300, etc.)
- No error handling tests for auth flows
- No integration-specific test cases
Recommendations:
- Create an auth strategy interface and concrete implementations for each integration
- Move integration-specific logic to respective services
- Enable and expand test coverage for:
- src/app/auth/login/login.component.spec.ts
- src/app/core/services/qbd-direct/qbd-direct-core/qbd-direct-auth.service.spec.ts
- Other integration auth services
🔗 Analysis chain
Consider refactoring authentication logic for better maintainability.
The component is handling multiple integration authentications with similar patterns. Consider these architectural improvements:
- Extract the integration-specific authentication logic into a separate service
- Use a strategy pattern or factory pattern to handle different integration types
- Add unit tests for the new QBD Direct integration path
This would:
- Reduce component complexity
- Make it easier to add new integrations
- Improve testability
- Centralize error handling
Let's verify the test coverage:
Let me gather more information about the test coverage and implementation details.
Let me check the actual implementation of the login component and auth services to better understand the current architecture.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for QBD Direct related test files
rg -g "*.spec.ts" "QbdDirectAuthService|QBD_DIRECT"
Length of output: 574
Script:
#!/bin/bash
# Check the content of the QBD Direct auth service test file
cat src/app/core/services/qbd-direct/qbd-direct-core/qbd-direct-auth.service.spec.ts
# Search for login component test file
fd "login.component.spec.ts" --exec cat {}
# Look for other auth-related test files for comparison
rg -g "*.spec.ts" "describe.*Auth" -A 2
Length of output: 5406
Script:
#!/bin/bash
# Check login component implementation
cat src/app/auth/login/login.component.ts
# Check QbdDirectAuthService implementation
cat src/app/core/services/qbd-direct/qbd-direct-core/qbd-direct-auth.service.ts
# Check common auth service implementation
cat src/app/core/services/common/auth.service.ts
Length of output: 9246
ashwin1111
added
the
staging_deploy
2 participants
Description
Please add PR description here, add screenshots if needed
Clickup
https://app.clickup.com/
Summary by CodeRabbit
Release Notes
New Features
QBD_DIRECT_API_URLfor enhanced configuration.Bug Fixes
Documentation
QBD_DIRECTproperty.