feat: UI for the new "Conencted" badge #1144
Merged
Conversation
This file contains 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
WalkthroughThe pull request introduces a new Changes
Possibly related PRs
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 (
|
ashwin1111
approved these changes
Jan 9, 2025
* 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)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
src/app/core/services/common/integrations.service.ts (1)
12-16: Remove empty constructor block.The empty constructor block can be removed since TypeScript automatically creates these parameters as class properties when using the access modifier in the constructor parameters.
export class IntegrationsService { - constructor( - private apiService: ApiService, - private helper: HelperService - ) { - } + constructor( + private apiService: ApiService, + private helper: HelperService + ) {}src/app/core/models/integrations/integrations.model.ts (1)
3-12: Add JSDoc documentation for Integration type.The Integration type properties need documentation for better clarity and maintainability.
+/** + * Represents an integration between the application and a third-party service + */ export type Integration = { + /** Unique identifier for the integration */ id: number; + /** Organization identifier */ org_id: string; + /** Organization display name */ org_name: string; + /** Third-party application identifier */ tpa_id: string; + /** Third-party application display name */ tpa_name: string; + /** Type of integration (e.g., 'accounting', 'hrms') */ type: string; + /** Whether the integration is currently active */ is_active: boolean; + /** Whether the integration is in beta testing phase */ is_beta: boolean; }src/app/integrations/integrations-routing.module.ts (1)
17-19: Maintain consistent route naming convention.The route paths use inconsistent naming conventions:
- Some routes use hyphens:
bamboo-hr,business-central,qbd-direct- Others use underscores:
landing_v2Consider standardizing to either hyphens or underscores across all routes.
- path: 'landing_v2', + path: 'landing-v2', component: LandingV2Componentsrc/app/integrations/landing-v2/landing-v2.component.ts (1)
168-170: Remove unnecessary optional chaining.If we initialize the
connectedAppsarray as suggested earlier, the optional chaining operator isn't needed.- return this.connectedApps?.includes(appKey); + return this.connectedApps.includes(appKey);src/app/integrations/landing-v2/landing-v2.component.html (1)
46-50: Consider extracting the connection status display into a reusable component.The connection status display pattern is repeated across all integration cards. This could be extracted into a reusable component to improve maintainability and reduce duplication.
Create a new component
IntegrationConnectionStatus:@Component({ selector: 'app-integration-connection-status', template: ` <app-badge *ngIf="isConnected" text="Connected" [theme]="ThemeOption.SUCCESS"></app-badge> <button *ngIf="!isConnected" class="btn-connect">Connect</button> ` }) export class IntegrationConnectionStatus { @Input() isConnected: boolean; ThemeOption = ThemeOption; }Then simplify the usage in the template:
-@if (isAppConnected('NETSUITE')) { - <app-badge text="Connected" [theme]="ThemeOption.SUCCESS"></app-badge> -} @else { - <button class="btn-connect">Connect</button> -} +<app-integration-connection-status [isConnected]="isAppConnected('NETSUITE')"> +</app-integration-connection-status>Also applies to: 66-70, 85-89, 104-108, 123-127, 143-147, 161-165, 180-184, 200-204, 221-225
📜 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 (6)
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/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)
🔇 Additional comments (4)
src/app/integrations/landing-v2/landing-v2.component.ts (2)
80-91: LGTM! Well-structured integration mapping.The mapping is comprehensive, type-safe, and immutable. Good use of TypeScript's Record type and readonly modifier.
250-253: LGTM! Proper initialization sequence.The component correctly initializes both the login watcher and connected apps state.
src/app/integrations/landing-v2/landing-v2.component.html (2)
Line range hint
15-35: LGTM! Clean implementation of conditional tab rendering.The tabs are properly controlled by the
exposeC1Appsflag and maintain consistent styling.
Line range hint
37-39: LGTM! Well-structured responsive grid layout.The grid layout is properly responsive with appropriate breakpoints for different screen sizes.
Comment on lines
+5
to
+16
| xdescribe('IntegrationsService', () => { | ||
| let service: IntegrationsService; | ||
|
|
||
| beforeEach(() => { | ||
| TestBed.configureTestingModule({}); | ||
| service = TestBed.inject(IntegrationsService); | ||
| }); | ||
|
|
||
| it('should be created', () => { | ||
| expect(service).toBeTruthy(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Enable tests and add coverage for getIntegrations method.
The test suite has the following issues:
- Tests are disabled with
xdescribe - Missing test coverage for
getIntegrationsmethod - No error handling tests
- No API response mocking
Here's how to improve the test coverage:
-xdescribe('IntegrationsService', () => {
+describe('IntegrationsService', () => {
let service: IntegrationsService;
+ let apiService: jasmine.SpyObj<ApiService>;
+ let helperService: jasmine.SpyObj<HelperService>;
beforeEach(() => {
+ const apiSpy = jasmine.createSpyObj('ApiService', ['get']);
+ const helperSpy = jasmine.createSpyObj('HelperService', ['setBaseApiURL']);
TestBed.configureTestingModule({
- {}
+ providers: [
+ IntegrationsService,
+ { provide: ApiService, useValue: apiSpy },
+ { provide: HelperService, useValue: helperSpy }
+ ]
});
service = TestBed.inject(IntegrationsService);
+ apiService = TestBed.inject(ApiService) as jasmine.SpyObj<ApiService>;
+ helperService = TestBed.inject(HelperService) as jasmine.SpyObj<HelperService>;
});
it('should be created', () => {
expect(service).toBeTruthy();
});
+
+ describe('getIntegrations', () => {
+ it('should set base URL and return integrations', (done) => {
+ const mockIntegrations: Integration[] = [
+ {
+ id: 1,
+ org_id: 'org1',
+ org_name: 'Org 1',
+ tpa_id: 'tpa1',
+ tpa_name: 'TPA 1',
+ type: 'type1',
+ is_active: true,
+ is_beta: false
+ }
+ ];
+
+ helperService.setBaseApiURL.and.returnValue(undefined);
+ apiService.get.and.returnValue(of(mockIntegrations));
+
+ service.getIntegrations().subscribe({
+ next: (integrations) => {
+ expect(helperService.setBaseApiURL).toHaveBeenCalledWith(AppUrl.INTEGRATION);
+ expect(apiService.get).toHaveBeenCalledWith('/integrations/', {});
+ expect(integrations).toEqual(mockIntegrations);
+ done();
+ },
+ error: done.fail
+ });
+ });
+
+ it('should handle API errors', (done) => {
+ const errorResponse = new HttpErrorResponse({
+ error: 'test 404 error',
+ status: 404,
+ statusText: 'Not Found'
+ });
+
+ helperService.setBaseApiURL.and.returnValue(undefined);
+ apiService.get.and.returnValue(throwError(() => errorResponse));
+
+ service.getIntegrations().subscribe({
+ next: () => done.fail('should have failed'),
+ error: (error) => {
+ expect(error.status).toBe(404);
+ done();
+ }
+ });
+ });
+ });
});Committable suggestion skipped: line range outside the PR's diff.
Comment on lines
+18
to
+21
| getIntegrations(): Observable<Integration[]> { | ||
| this.helper.setBaseApiURL(AppUrl.INTEGRATION); | ||
| return this.apiService.get(`/integrations/`, {}); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and retry logic.
The getIntegrations method should:
- Handle API errors gracefully
- Implement retry logic for transient failures
- Consider caching the results to improve performance
getIntegrations(): Observable<Integration[]> {
this.helper.setBaseApiURL(AppUrl.INTEGRATION);
- return this.apiService.get(`/integrations/`, {});
+ return this.apiService.get<Integration[]>('/integrations/', {}).pipe(
+ retry({
+ count: 3,
+ delay: (error, retryCount) => {
+ if (error.status === 404) {
+ return EMPTY;
+ }
+ return timer(1000 * retryCount);
+ }
+ }),
+ catchError((error: HttpErrorResponse) => {
+ console.error('Error fetching integrations:', error);
+ return throwError(() => 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.
Suggested change
| getIntegrations(): Observable<Integration[]> { | |
| this.helper.setBaseApiURL(AppUrl.INTEGRATION); | |
| return this.apiService.get(`/integrations/`, {}); | |
| } | |
| getIntegrations(): Observable<Integration[]> { | |
| this.helper.setBaseApiURL(AppUrl.INTEGRATION); | |
| return this.apiService.get<Integration[]>('/integrations/', {}).pipe( | |
| retry({ | |
| count: 3, | |
| delay: (error, retryCount) => { | |
| if (error.status === 404) { | |
| return EMPTY; | |
| } | |
| return timer(1000 * retryCount); | |
| } | |
| }), | |
| catchError((error: HttpErrorResponse) => { | |
| console.error('Error fetching integrations:', error); | |
| return throwError(() => error); | |
| }) | |
| ); | |
| } |
Comment on lines
+241
to
+248
| 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.
Add error handling and cleanup subscription.
The subscription to getIntegrations() should handle errors and be cleaned up to prevent memory leaks.
+ private destroy$ = new Subject<void>();
private storeConnectedApps() {
- this.integrationService.getIntegrations().subscribe(integrations => {
+ this.integrationService.getIntegrations().pipe(
+ takeUntil(this.destroy$),
+ catchError(error => {
+ console.error('Failed to fetch integrations:', error);
+ return of([]);
+ })
+ ).subscribe(integrations => {
const tpaNames = integrations.map(integration => integration.tpa_name);
const connectedApps = tpaNames.map(tpaName => this.tpaNameToIntegrationKeyMap[tpaName]);
this.connectedApps = connectedApps;
});
}Don't forget to implement ngOnDestroy:
ngOnDestroy(): void {
this.destroy$.next();
this.destroy$.complete();
}| @@ -31,6 +32,10 @@ | |||
|
|
|||
| org: Org = this.orgService.getCachedOrg(); | |||
|
|
|||
| private connectedApps: IntegrationAppKey[]; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Initialize the connectedApps array to prevent undefined access.
The connectedApps array should be initialized to prevent potential undefined access in the isAppConnected method.
- private connectedApps: IntegrationAppKey[];
+ private connectedApps: IntegrationAppKey[] = [];📝 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
| private connectedApps: IntegrationAppKey[]; | |
| private connectedApps: IntegrationAppKey[] = []; |
JustARatherRidiculouslyLongUsername
merged commit 84ceb24
into
logos-and-hover
5 checks passed
JustARatherRidiculouslyLongUsername
added a commit
that referenced
this pull request
Jan 17, 2025
* 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)
JustARatherRidiculouslyLongUsername
added a commit
that referenced
this pull request
Jan 17, 2025
* 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)
JustARatherRidiculouslyLongUsername
added a commit
that referenced
this pull request
Jan 17, 2025
* 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)
JustARatherRidiculouslyLongUsername
added a commit
that referenced
this pull request
Jan 17, 2025
* 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)
JustARatherRidiculouslyLongUsername
added a commit
that referenced
this pull request
Jan 17, 2025
* 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)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
https://app.clickup.com/t/86cxhewex
Summary by CodeRabbit
New Features
SUCCESStheme option to the existing theme enum..theme-successCSS class for badges with success styling.IntegrationsServicefor managing integration data retrieval.LandingV2Componentto display connection status for various applications.Bug Fixes
LandingV2Componentto use underscores instead of hyphens.Tests
IntegrationsService.