feat: fetch data from integrations API and show connected apps #1143
Conversation
WalkthroughThis pull request introduces a new Changes
Suggested labels
Suggested reviewers
Poem
Finishing Touches
🪧 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
🧹 Nitpick comments (6)
src/app/core/services/common/integrations.service.ts (1)
12-17: Consider moving base URL configuration to a more appropriate locationSetting the base URL in the constructor couples the service initialization with side effects. Consider moving this configuration to a separate initialization method or to a higher level in the application.
src/app/core/models/integrations/integrations.model.ts (1)
3-12: Add JSDoc documentation and consider more specific typesThe Integration type would benefit from documentation explaining each field's purpose. Also, consider using more specific types for certain fields.
+/** + * Represents an integration between Fyle and a third-party application + */ export type Integration = { id: number; org_id: string; org_name: string; tpa_id: string; tpa_name: string; - type: string; + type: 'ACCOUNTING' | 'HRMS' | 'TRAVEL'; is_active: boolean; is_beta: boolean; }src/app/integrations/landing-v2/landing-v2.component.ts (1)
78-89: Consider moving integration mappings to a configuration fileThe
tpaNameToIntegrationKeyMapcontains hard-coded strings that would be better maintained in a separate configuration file. This would improve maintainability and reduce the risk of typos.src/app/integrations/landing-v2/landing-v2.component.html (3)
44-50: Enhance accessibility for connection status indicatorsWhile the UI changes effectively show connection status, consider improving accessibility by:
- Adding ARIA labels to indicate connection status
- Ensuring the Connect buttons have proper focus states
- Adding proper role attributes to the status badges
Example implementation for one block:
-<button class="btn-connect">Connect</button> +<button + class="btn-connect" + aria-label="Connect to NetSuite" +>Connect</button> -<app-badge text="Connected" [theme]="ThemeOption.SUCCESS"></app-badge> +<app-badge + text="Connected" + [theme]="ThemeOption.SUCCESS" + role="status" + aria-label="NetSuite is connected" +></app-badge>Also applies to: 64-70, 83-89, 102-108, 121-127, 141-147, 159-165, 178-184, 198-204, 219-225
46-48: Consider caching connection status checksThe template calls
isAppConnected()multiple times for each integration. Consider caching these results to optimize performance:
- Create a map of connection statuses in the component
private connectionStatusMap = new Map<string, boolean>(); ngOnInit() { this.storeConnectedApps().subscribe(() => { // Cache connection status for each app ['NETSUITE', 'INTACCT', 'QBO', /* ... */].forEach(app => { this.connectionStatusMap.set(app, this.isAppConnected(app)); }); }); }
- Use the cached values in template:
-@if (isAppConnected('NETSUITE')) { +@if (connectionStatusMap.get('NETSUITE')) {Also applies to: 66-67, 85-86, 104-105, 123-124, 143-144, 161-162, 180-181, 200-201, 221-222
Line range hint
173-175: Simplify beta badge managementThe current implementation uses separate arrays (
orgsToHideSage300BetaBadge,orgsToHideBusinessCentralBetaBadge) to manage beta badge visibility. Consider a more maintainable approach:
- Define a single configuration object:
interface IntegrationConfig { id: string; name: string; type: 'Accounting' | 'HRMS' | 'Travel'; isBeta?: boolean; betaExcludedOrgs?: string[]; } const INTEGRATIONS_CONFIG: IntegrationConfig[] = [ { id: 'SAGE300', name: 'Sage 300 CRE', type: 'Accounting', isBeta: true, betaExcludedOrgs: ['org1', 'org2'] }, // ... other integrations ];
- Create a helper method:
shouldShowBetaBadge(integrationId: string): boolean { const config = INTEGRATIONS_CONFIG.find(i => i.id === integrationId); return config?.isBeta && !config?.betaExcludedOrgs?.includes(this.org.fyle_org_id); }
- Update template:
-<app-badge *ngIf="!orgsToHideSage300BetaBadge.includes(org.fyle_org_id)" +<app-badge *ngIf="shouldShowBetaBadge('SAGE300')" [theme]="ThemeOption.DARK" text="Beta"> </app-badge>Also applies to: 191-194
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/app/core/models/integrations/integrations.model.ts(1 hunks)src/app/core/services/common/integrations.service.spec.ts(1 hunks)src/app/core/services/common/integrations.service.ts(1 hunks)src/app/integrations/landing-v2/landing-v2.component.html(10 hunks)src/app/integrations/landing-v2/landing-v2.component.ts(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: unit-test
| it('should be created', () => { | ||
| expect(service).toBeTruthy(); | ||
| }); |
There was a problem hiding this comment.
Add test coverage for getIntegrations method
The test suite only verifies service creation. Add tests for the getIntegrations method to ensure it correctly fetches and returns integration data.
it('should fetch integrations', (done) => {
const mockIntegrations = [
{
id: 1,
org_id: 'org1',
org_name: 'Org 1',
tpa_id: 'tpa1',
tpa_name: 'TPA 1',
type: 'type1',
is_active: true,
is_beta: false
}
];
const apiService = TestBed.inject(ApiService) as jasmine.SpyObj<ApiService>;
apiService.get.and.returnValue(of(mockIntegrations));
service.getIntegrations().subscribe(integrations => {
expect(apiService.get).toHaveBeenCalledWith('/integrations/', {});
expect(integrations).toEqual(mockIntegrations);
done();
});
});|
|
||
| import { IntegrationsService } from './integrations.service'; | ||
|
|
||
| xdescribe('IntegrationsService', () => { |
There was a problem hiding this comment.
Enable the test suite by removing the 'x' prefix
The test suite is currently disabled with xdescribe. Enable it by changing to describe.
-xdescribe('IntegrationsService', () => {
+describe('IntegrationsService', () => {📝 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.
| xdescribe('IntegrationsService', () => { | |
| describe('IntegrationsService', () => { |
| beforeEach(() => { | ||
| TestBed.configureTestingModule({}); | ||
| service = TestBed.inject(IntegrationsService); | ||
| }); |
There was a problem hiding this comment.
Add missing ApiService provider in TestBed configuration
The service depends on ApiService but it's not provided in the testing module.
beforeEach(() => {
- TestBed.configureTestingModule({});
+ TestBed.configureTestingModule({
+ providers: [
+ IntegrationsService,
+ {
+ provide: ApiService,
+ useValue: jasmine.createSpyObj('ApiService', ['get'])
+ }
+ ]
+ });
service = TestBed.inject(IntegrationsService);
});📝 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.
| beforeEach(() => { | |
| TestBed.configureTestingModule({}); | |
| service = TestBed.inject(IntegrationsService); | |
| }); | |
| beforeEach(() => { | |
| TestBed.configureTestingModule({ | |
| providers: [ | |
| IntegrationsService, | |
| { | |
| provide: ApiService, | |
| useValue: jasmine.createSpyObj('ApiService', ['get']) | |
| } | |
| ] | |
| }); | |
| service = TestBed.inject(IntegrationsService); | |
| }); |
| getIntegrations(): Observable<Integration[]> { | ||
| return this.apiService.get(`/integrations/`, {}); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for API requests
The getIntegrations method should handle potential API errors to prevent them from propagating to components.
getIntegrations(): Observable<Integration[]> {
- return this.apiService.get(`/integrations/`, {});
+ return this.apiService.get(`/integrations/`, {}).pipe(
+ catchError(error => {
+ console.error('Error fetching integrations:', error);
+ return throwError(() => new Error('Failed to fetch integrations'));
+ })
+ );
}Committable suggestion skipped: line range outside the PR's diff.
| private storeConnectedApps() { | ||
| this.integrationService.getIntegrations().subscribe(integrations => { | ||
| const tpaNames = integrations.map(integration => integration.tpa_name); | ||
| const connectedApps = tpaNames.map(tpaName => this.tpaNameToIntegrationKeyMap[tpaName]); | ||
|
|
||
| this.connectedApps = connectedApps; | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and loading state for integration fetching
The storeConnectedApps method lacks error handling and doesn't manage loading state. This could lead to silent failures and poor user experience.
+private isLoading = false;
+private error: string | null = null;
private storeConnectedApps() {
+ this.isLoading = true;
+ this.error = null;
this.integrationService.getIntegrations().subscribe({
- integrations => {
+ next: (integrations) => {
const tpaNames = integrations.map(integration => integration.tpa_name);
const connectedApps = tpaNames.map(tpaName => this.tpaNameToIntegrationKeyMap[tpaName]);
this.connectedApps = connectedApps;
+ this.isLoading = false;
+ },
+ error: (error) => {
+ console.error('Failed to fetch integrations:', error);
+ this.error = 'Failed to load connected apps';
+ this.isLoading = false;
}
});
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private storeConnectedApps() { | |
| this.integrationService.getIntegrations().subscribe(integrations => { | |
| const tpaNames = integrations.map(integration => integration.tpa_name); | |
| const connectedApps = tpaNames.map(tpaName => this.tpaNameToIntegrationKeyMap[tpaName]); | |
| this.connectedApps = connectedApps; | |
| }); | |
| } | |
| private isLoading = false; | |
| private error: string | null = null; | |
| private storeConnectedApps() { | |
| this.isLoading = true; | |
| this.error = null; | |
| this.integrationService.getIntegrations().subscribe({ | |
| next: (integrations) => { | |
| const tpaNames = integrations.map(integration => integration.tpa_name); | |
| const connectedApps = tpaNames.map(tpaName => this.tpaNameToIntegrationKeyMap[tpaName]); | |
| this.connectedApps = connectedApps; | |
| this.isLoading = false; | |
| }, | |
| error: (error) => { | |
| console.error('Failed to fetch integrations:', error); | |
| this.error = 'Failed to load connected apps'; | |
| this.isLoading = false; | |
| } | |
| }); | |
| } |
* fix: update route name and keep `exposeC1Apps` * fix: use new logos in landing-v2 only Use original images everywhere else. * fix: incorrect URL for `/integrations` call and rendering errors (#1149)
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/app/integrations/landing-v2/landing-v2.component.ts (3)
80-91: 🛠️ Refactor suggestionUse enum for TPA names instead of string literals
Based on the past review comments and team's preference, consider creating an enum for TPA names instead of using string literals. This would provide better type safety and maintainability.
Example implementation:
enum TpaName { NETSUITE = 'Fyle Netsuite Integration', SAGE_INTACCT = 'Fyle Sage Intacct Integration', // ... other TPA names } private readonly tpaNameToIntegrationKeyMap: Record<TpaName, IntegrationAppKey> = { [TpaName.NETSUITE]: 'NETSUITE', [TpaName.SAGE_INTACCT]: 'INTACCT', // ... other mappings };
168-170:⚠️ Potential issueComplete the null check implementation
While optional chaining is used, the method should also handle the case when the check returns undefined.
- return this.connectedApps?.includes(appKey); + return this.connectedApps?.includes(appKey) ?? false;
241-248:⚠️ Potential issueAdd error handling and loading state
The method needs proper error handling and loading state management as suggested in the past review.
Additionally, add type safety to the mapping operation:
private storeConnectedApps() { this.isLoading = true; this.error = null; this.integrationService.getIntegrations().subscribe({ next: (integrations) => { const tpaNames = integrations.map(integration => integration.tpa_name); - const connectedApps = tpaNames.map(tpaName => this.tpaNameToIntegrationKeyMap[tpaName]); + const connectedApps = tpaNames + .map(tpaName => this.tpaNameToIntegrationKeyMap[tpaName]) + .filter((appKey): appKey is IntegrationAppKey => appKey !== undefined); this.connectedApps = connectedApps; this.isLoading = false; }, error: (error) => { console.error('Failed to fetch integrations:', error); this.error = 'Failed to load connected apps'; this.isLoading = false; } }); }
🧹 Nitpick comments (1)
src/app/integrations/landing-v2/landing-v2.component.ts (1)
251-252: Consider using a resolver or guard for initializationThe
storeConnectedAppscall should be protected to ensure the service is ready and the component is properly initialized.Consider using a resolver:
@Injectable() export class IntegrationsResolver implements Resolve<void> { constructor(private integrationService: IntegrationsService) {} resolve(): Observable<void> { return this.integrationService.getIntegrations().pipe( map(() => undefined), catchError(() => EMPTY) ); } }Then update the route configuration:
{ path: 'landing_v2', component: LandingV2Component, resolve: { integrations: IntegrationsResolver } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
src/assets/logos/intacct-logo-new.pngis excluded by!**/*.pngsrc/assets/logos/intacct-logo.pngis excluded by!**/*.pngsrc/assets/logos/netsuite-logo-new.pngis excluded by!**/*.pngsrc/assets/logos/netsuite-logo.pngis excluded by!**/*.pngsrc/assets/logos/xero-logo-new.pngis excluded by!**/*.pngsrc/assets/logos/xero-logo.pngis excluded by!**/*.png
📒 Files selected for processing (4)
src/app/core/services/common/integrations.service.ts(1 hunks)src/app/integrations/integrations-routing.module.ts(1 hunks)src/app/integrations/landing-v2/landing-v2.component.html(11 hunks)src/app/integrations/landing-v2/landing-v2.component.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/app/integrations/landing-v2/landing-v2.component.html
- src/app/core/services/common/integrations.service.ts
🔇 Additional comments (2)
src/app/integrations/integrations-routing.module.ts (1)
17-17: LGTM! Route path updated for consistencyThe change from hyphen to underscore in the route path aligns with the naming convention used in other routes within this module.
src/app/integrations/landing-v2/landing-v2.component.ts (1)
121-122: LGTM! Dependency injection is properly implementedThe IntegrationsService is correctly injected as a private dependency.
JustARatherRidiculouslyLongUsername
merged commit be16dd3
into
connected-badge-ui
* feat: UI for the new "Conencted" badge * feat: fetch data from integrations API and show connected apps (#1143) * feat: fetch data from integrations API and show connected apps * fix: update route name and keep `exposeC1Apps` (#1145) * fix: update route name and keep `exposeC1Apps` * fix: use new logos in landing-v2 only Use original images everywhere else. * fix: incorrect URL for `/integrations` call and rendering errors (#1149)
* fix: update images * feat: add CTA and shadow animation on tile hover * feat: UI for the new "Conencted" badge (#1144) * feat: UI for the new "Conencted" badge * feat: fetch data from integrations API and show connected apps (#1143) * feat: fetch data from integrations API and show connected apps * fix: update route name and keep `exposeC1Apps` (#1145) * fix: update route name and keep `exposeC1Apps` * fix: use new logos in landing-v2 only Use original images everywhere else. * fix: incorrect URL for `/integrations` call and rendering errors (#1149)
* feat: responsive grid layout + new tile layout * feat: add CTA and shadow animation on tile hover (#1142) * fix: update images * feat: add CTA and shadow animation on tile hover * feat: UI for the new "Conencted" badge (#1144) * feat: UI for the new "Conencted" badge * feat: fetch data from integrations API and show connected apps (#1143) * feat: fetch data from integrations API and show connected apps * fix: update route name and keep `exposeC1Apps` (#1145) * fix: update route name and keep `exposeC1Apps` * fix: use new logos in landing-v2 only Use original images everywhere else. * fix: incorrect URL for `/integrations` call and rendering errors (#1149)
* feat: new app tile structure + remove extra content * feat: responsive grid layout + new tile layout (#1141) * feat: responsive grid layout + new tile layout * feat: add CTA and shadow animation on tile hover (#1142) * fix: update images * feat: add CTA and shadow animation on tile hover * feat: UI for the new "Conencted" badge (#1144) * feat: UI for the new "Conencted" badge * feat: fetch data from integrations API and show connected apps (#1143) * feat: fetch data from integrations API and show connected apps * fix: update route name and keep `exposeC1Apps` (#1145) * fix: update route name and keep `exposeC1Apps` * fix: use new logos in landing-v2 only Use original images everywhere else. * fix: incorrect URL for `/integrations` call and rendering errors (#1149)
* feat: create new landing page and add header and tab switcher * feat: new app tile structure + remove extra content (#1140) * feat: new app tile structure + remove extra content * feat: responsive grid layout + new tile layout (#1141) * feat: responsive grid layout + new tile layout * feat: add CTA and shadow animation on tile hover (#1142) * fix: update images * feat: add CTA and shadow animation on tile hover * feat: UI for the new "Conencted" badge (#1144) * feat: UI for the new "Conencted" badge * feat: fetch data from integrations API and show connected apps (#1143) * feat: fetch data from integrations API and show connected apps * fix: update route name and keep `exposeC1Apps` (#1145) * fix: update route name and keep `exposeC1Apps` * fix: use new logos in landing-v2 only Use original images everywhere else. * fix: incorrect URL for `/integrations` call and rendering errors (#1149)
* feat: create new landing page and add header and tab switcher * feat: new app tile structure + remove extra content (#1140) * feat: new app tile structure + remove extra content * feat: responsive grid layout + new tile layout (#1141) * feat: responsive grid layout + new tile layout * feat: add CTA and shadow animation on tile hover (#1142) * fix: update images * feat: add CTA and shadow animation on tile hover * feat: UI for the new "Conencted" badge (#1144) * feat: UI for the new "Conencted" badge * feat: fetch data from integrations API and show connected apps (#1143) * feat: fetch data from integrations API and show connected apps * fix: update route name and keep `exposeC1Apps` (#1145) * fix: update route name and keep `exposeC1Apps` * fix: use new logos in landing-v2 only Use original images everywhere else. * fix: incorrect URL for `/integrations` call and rendering errors (#1149)
Clickup
https://app.clickup.com/t/86cxhewex
Summary by CodeRabbit
Release Notes
New Features
Improvements
Technical Updates
The updates provide a more dynamic and informative experience for managing integrations, with real-time connection status tracking.