Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add CTA and shadow animation on tile hover #1142

Merged

Conversation

JustARatherRidiculouslyLongUsername
Copy link
Contributor

@JustARatherRidiculouslyLongUsername JustARatherRidiculouslyLongUsername commented Jan 7, 2025

Clickup

https://app.clickup.com/t/86cxhz07t

Summary by CodeRabbit

  • New Features

    • Added "Connect" buttons to integration application cards.
    • Introduced new success theme option for badges.
    • Implemented functionality to check and store connected applications.
  • Style

    • Updated styling for integration app cards and added hover effects.
    • Enhanced visual styling for success-themed badges.
  • User Experience

    • Improved card layout with horizontal logo and connection status.
    • Enhanced interactivity with hover states for integration app cards.

Copy link
Contributor

coderabbitai bot commented Jan 7, 2025

Walkthrough

The pull request modifies the layout and functionality of integration application cards in the LandingV2Component. A conditional check for exposeC1Apps has been added to control the visibility of integration tabs. The application cards now feature a new structure that includes a "Connected" badge or a "Connect" button based on the connection status. Additionally, a new IntegrationsService is introduced to handle integration data retrieval, and a new Integration type is defined. The routing for the LandingV2Component has also been updated.

Changes

File Change Summary
src/app/integrations/landing-v2/landing-v2.component.html - Added conditional rendering for integration tabs
- Updated structure for application cards with connection status indicators
- Changed some logo image sources from .svg to .png
src/app/integrations/landing-v2/landing-v2.component.scss - Updated styling for .landing-v2--accounting-app with hover effects
- Introduced .btn-connect class with hidden default state
src/app/core/models/enum/enum.model.ts - Added SUCCESS value to ThemeOption enum
src/app/core/models/integrations/integrations.model.ts - Added new Integration type with multiple properties
src/app/core/services/common/integrations.service.ts - Introduced IntegrationsService with getIntegrations method
src/app/core/services/common/integrations.service.spec.ts - Added test suite for IntegrationsService
src/app/integrations/integrations-routing.module.ts - Modified routing path for LandingV2Component from landing-v2 to landing_v2
src/app/integrations/landing-v2/landing-v2.component.ts - Added properties and methods for managing connected applications
- Implemented logic to retrieve and store connected applications
src/app/shared/components/core/badge/badge.component.scss - Added .theme-success class for success-themed badges

Possibly related PRs

Suggested labels

deploy

Suggested reviewers

  • ashwin1111

Poem

🐰 Integrations hop and dance,

With buttons that now enhance!

Connect with just a hover's grace,

Our logos find their perfect place

A rabbit's tech-design romance! 🌟

Finishing Touches

  • 📝 Generate Docstrings (Beta)

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🔭 Outside diff range comments (1)
src/app/integrations/landing-v2/landing-v2.component.html (1)

Accessibility issues confirmed - keyboard navigation needs improvement

The current implementation has confirmed accessibility issues:

  1. The connect button is only visible on hover, making it inaccessible to keyboard users
  2. No focus states are defined in the SCSS
  3. No keyboard navigation handlers are implemented

Required changes:

  • Add tabindex="0" to clickable elements
  • Make the connect button visible on both hover and focus
  • Implement keyboard handlers for activation (Enter/Space)
  • Add focus styles as suggested in the original review

Example SCSS update:

.landing-v2--accounting-app {
  &:hover,
  &:focus-within {
    box-shadow: 0px 4px 4px 0px rgba(44, 48, 78, 0.10);
    .btn-connect {
      @apply tw-block;
    }
  }

  &:focus-visible {
    @apply tw-outline-2 tw-outline-offset-2 tw-outline-primary;
  }
}
🔗 Analysis chain

Line range hint 1-199: Enhance keyboard navigation and focus states

The current implementation might have accessibility issues:

  1. Keyboard navigation path isn't clear with the new button layout
  2. Focus states aren't explicitly defined
  3. Color contrast for hover states should be verified

Add the following improvements:

  1. Ensure the button is keyboard focusable
  2. Add focus-visible styles
  3. Verify color contrast ratios

Example focus styles to add to your SCSS:

.btn-connect {
  @apply tw-transition-all;
  
  &:focus-visible {
    @apply tw-outline-2 tw-outline-offset-2 tw-outline-primary;
  }
}

.landing-v2--accounting-app {
  &:focus-within {
    @apply tw-ring-2 tw-ring-primary tw-ring-offset-2;
  }
}

Run this command to check color contrast ratios:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Find all color definitions in the SCSS files
rg -g '*.scss' '(color|background-color|border-color):\s*#[0-9a-fA-F]{3,6}' ./src

Length of output: 2028


Script:

#!/bin/bash
# Check component SCSS and keyboard navigation implementation
echo "=== Landing V2 Component SCSS ==="
fd -e scss "landing-v2" -x cat {}

echo -e "\n=== Keyboard Navigation Related Code ==="
rg -g "*.ts" "keydown|keyup|tabindex|focus|blur" ./src/app/integrations/landing-v2/

Length of output: 2127

🧹 Nitpick comments (4)
src/app/integrations/landing-v2/landing-v2.component.scss (4)

26-27: Consider using Tailwind's transition classes consistently

The shadow transition could be more maintainable using Tailwind's built-in transition utilities.

-@apply tw-transition-shadow;
+@apply tw-transition-all tw-duration-200 tw-ease-in-out;

67-71: Consider using Tailwind's shadow utilities

Instead of hardcoding shadow values, consider using Tailwind's built-in shadow utilities for better maintainability and consistency.

-.landing-v2--accounting-app:hover {
-    box-shadow: 0px 4px 4px 0px rgba(44, 48, 78, 0.10);
+.landing-v2--accounting-app:hover {
+    @apply tw-shadow-md;

Line range hint 44-47: Consider extracting repeated button structure into a component

The same button structure is repeated across multiple integration tiles. This could be extracted into a reusable component for better maintainability.

Consider creating an integration-tile component that accepts the logo and other properties as inputs:

// integration-tile.component.ts
@Component({
  selector: 'app-integration-tile',
  template: `
    <div class="tw-flex tw-justify-between">
      <img [src]="logoSrc" [class]="logoClass" />
      <button class="btn-connect">Connect</button>
    </div>
  `
})
export class IntegrationTileComponent {
  @Input() logoSrc: string;
  @Input() logoClass?: string;
}

Also applies to: 60-63, 75-78, 90-93, 105-108, 121-124, 135-138, 150-153, 166-169, 183-186


Line range hint 76-76: Standardize image sizing approach

Different approaches are used for image sizing:

  • QuickBooks logo uses !tw-h-[30.7px]
  • Sage300 uses tw-py-[4px]
  • TravelPerk uses tw-py-[5px]

Consider standardizing the approach using a consistent method for all logos.

-<img src="assets/logos/quickbooks-logo.png" class="!tw-h-[30.7px]" />
+<img src="assets/logos/quickbooks-logo.png" class="tw-h-[40px] tw-object-contain" />

Also applies to: 106-106, 122-122

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8039c21 and dd8b2e5.

⛔ Files ignored due to path filters (6)
  • src/assets/logos/bamboo-hr-logo.png is excluded by !**/*.png
  • src/assets/logos/intacct-logo.png is excluded by !**/*.png
  • src/assets/logos/netsuite-logo.png is excluded by !**/*.png
  • src/assets/logos/sage300-logo.png is excluded by !**/*.png
  • src/assets/logos/travelperk-logo.png is excluded by !**/*.png
  • src/assets/logos/xero-logo.png is excluded by !**/*.png
📒 Files selected for processing (2)
  • src/app/integrations/landing-v2/landing-v2.component.html (10 hunks)
  • src/app/integrations/landing-v2/landing-v2.component.scss (2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: unit-test
🔇 Additional comments (1)
src/app/integrations/landing-v2/landing-v2.component.scss (1)

33-35: LGTM! Clean implementation of the connect button

The button is properly hidden by default and follows the text size convention.

@@ -66,7 +72,10 @@ <h3 class="landing-v2--section-heading">
</div>
<div *ngIf="isAppShown('QBO')" class="landing-v2--accounting-app"
(click)="openAccountingIntegrationApp(AccountingIntegrationApp.QBO)">
<img src="assets/logos/quickbooks-logo.png" />
<div class="tw-flex tw-justify-between">
<img src="assets/logos/quickbooks-logo.png" class="!tw-h-[30.7px]" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Standardize image sizing approach

The current implementation uses inconsistent approaches for image sizing:

  • Some images use fixed heights: !tw-h-[30.7px]
  • Some use padding: tw-py-[4px], tw-py-[5px]
  • Others have no size constraints

Consider standardizing the approach:

  1. Create CSS classes for different logo size categories
  2. Use aspect-ratio to maintain proportions
  3. Remove magic numbers

Example:

-                    <img src="assets/logos/quickbooks-logo.png" class="!tw-h-[30.7px]" />
+                    <img src="assets/logos/quickbooks-logo.png" class="logo-img logo-img--medium" />

Add to your SCSS:

.logo-img {
  @apply tw-object-contain tw-w-auto;
  
  &--small {
    @apply tw-h-8;
  }
  
  &--medium {
    @apply tw-h-10;
  }
  
  &--large {
    @apply tw-h-12;
  }
}

Also applies to: 106-106, 122-122, 136-136, 151-151, 167-167, 184-184

Comment on lines 44 to 47
<div class="tw-flex tw-justify-between">
<img src="assets/logos/netsuite-logo.png" />
<button class="btn-connect">Connect</button>
</div>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Address click handler conflicts and improve accessibility

The current implementation has several issues:

  1. The parent div's click handler (openAccountingIntegrationApp) conflicts with the button's click functionality
  2. The button lacks accessibility attributes
  3. Users might be confused about whether to click the tile or the button

Consider this implementation:

-                <div class="landing-v2--accounting-app"
-                    (click)="openAccountingIntegrationApp(AccountingIntegrationApp.NETSUITE)">
-                    <div class="tw-flex tw-justify-between">
-                        <img src="assets/logos/netsuite-logo.png" />
-                        <button class="btn-connect">Connect</button>
+                <div class="landing-v2--accounting-app">
+                    <div class="tw-flex tw-justify-between">
+                        <img src="assets/logos/netsuite-logo.png" alt="NetSuite logo" />
+                        <button 
+                            class="btn-connect"
+                            (click)="openAccountingIntegrationApp(AccountingIntegrationApp.NETSUITE)"
+                            aria-label="Connect to NetSuite"
+                            role="button">
+                            Connect
+                        </button>
📝 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
<div class="tw-flex tw-justify-between">
<img src="assets/logos/netsuite-logo.png" />
<button class="btn-connect">Connect</button>
</div>
<div class="landing-v2--accounting-app">
<div class="tw-flex tw-justify-between">
<img src="assets/logos/netsuite-logo.png" alt="NetSuite logo" />
<button
class="btn-connect"
(click)="openAccountingIntegrationApp(AccountingIntegrationApp.NETSUITE)"
aria-label="Connect to NetSuite"
role="button">
Connect
</button>
</div>

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🔭 Outside diff range comments (1)
src/app/integrations/landing-v2/landing-v2.component.html (1)

Line range hint 60-186: Consider extracting integration card into a reusable component

The integration card pattern is repeated multiple times with similar structure. Consider creating a reusable component to:

  • Reduce code duplication
  • Centralize accessibility improvements
  • Simplify maintenance
  • Standardize image sizing

Example component interface:

interface IntegrationCardProps {
  name: string;
  type: 'Accounting' | 'HRMS' | 'Travel';
  logo: string;
  logoClass?: string;
  onClick: () => void;
  showBetaBadge?: boolean;
  betaBadgeHidden?: boolean;
}

This would simplify the template to:

<app-integration-card
  name="NetSuite"
  type="Accounting"
  logo="assets/logos/netsuite-logo.png"
  [onClick]="() => openAccountingIntegrationApp(AccountingIntegrationApp.NETSUITE)"
>
</app-integration-card>
🧹 Nitpick comments (3)
src/app/integrations/landing-v2/landing-v2.component.scss (2)

33-35: Consider adding hover state styling for the button

While the button visibility is properly handled, consider adding hover state styling for better interactivity.

 .btn-connect {
     @apply tw-text-14-px tw-hidden;
+    @apply hover:tw-bg-slightly-normal-background-color tw-px-3 tw-py-1 tw-rounded;
 }

67-71: Use Tailwind's shadow utilities for consistency

Consider using Tailwind's built-in shadow utilities instead of custom box-shadow for better maintainability and design system consistency.

 .landing-v2--accounting-app:hover {
-    box-shadow: 0px 4px 4px 0px rgba(44, 48, 78, 0.10);
+    @apply tw-shadow-md;
     .btn-connect {
         @apply tw-block;
     }
 }
src/app/integrations/landing-v2/landing-v2.component.html (1)

45-45: Maintain consistency in logo image formats

Some logos use PNG while others use SVG format. Consider using SVG for all logos to ensure:

  • Better scaling on high-DPI displays
  • Smaller file sizes
  • Consistent quality across all integrations

Also applies to: 61-61, 76-76, 91-91, 106-106, 122-122, 136-136, 151-151, 167-167, 184-184

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8039c21 and dd8b2e5.

⛔ Files ignored due to path filters (6)
  • src/assets/logos/bamboo-hr-logo.png is excluded by !**/*.png
  • src/assets/logos/intacct-logo.png is excluded by !**/*.png
  • src/assets/logos/netsuite-logo.png is excluded by !**/*.png
  • src/assets/logos/sage300-logo.png is excluded by !**/*.png
  • src/assets/logos/travelperk-logo.png is excluded by !**/*.png
  • src/assets/logos/xero-logo.png is excluded by !**/*.png
📒 Files selected for processing (2)
  • src/app/integrations/landing-v2/landing-v2.component.html (10 hunks)
  • src/app/integrations/landing-v2/landing-v2.component.scss (2 hunks)
🔇 Additional comments (1)
src/app/integrations/landing-v2/landing-v2.component.scss (1)

26-27: LGTM! Clean transition effect implementation

The transition effect and flex layout changes are well-implemented using Tailwind's utility classes.

Comment on lines 44 to 47
<div class="tw-flex tw-justify-between">
<img src="assets/logos/netsuite-logo.png" />
<button class="btn-connect">Connect</button>
</div>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add accessibility attributes and handle button click separately

  1. The button lacks accessibility attributes
  2. The button click might trigger the parent's click handler unintentionally
-                    <button class="btn-connect">Connect</button>
+                    <button 
+                        class="btn-connect" 
+                        aria-label="Connect to NetSuite"
+                        (click)="$event.stopPropagation(); openAccountingIntegrationApp(AccountingIntegrationApp.NETSUITE)"
+                    >
+                        Connect
+                    </button>

Committable suggestion skipped: line range outside the PR's diff.

* 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)
Copy link

Unit Test Coverage % values
Statements 33.01% ( 4121 / 12483 )
Branches 26.34% ( 1175 / 4460 )
Functions 25.6% ( 893 / 3487 )
Lines 33.19% ( 4055 / 12216 )

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Nitpick comments (3)
src/app/core/models/integrations/integrations.model.ts (1)

3-12: Enhance type safety and add documentation

The Integration type could benefit from:

  1. JSDoc comments explaining each field
  2. Stricter typing for specific fields
  3. Readonly modifiers for immutable data

Here's the improved type definition:

+/**
+ * Represents an integration between the application and a third-party service
+ */
 export type Integration = {
-    id: number;
-    org_id: string;
-    org_name: string;
-    tpa_id: string;
-    tpa_name: string;
-    type: string;
-    is_active: boolean;
-    is_beta: boolean;
+    /** Unique identifier for the integration */
+    readonly id: number;
+    /** Organization identifier */
+    readonly org_id: string;
+    /** Organization display name */
+    readonly org_name: string;
+    /** Third-party application identifier */
+    readonly tpa_id: string;
+    /** Third-party application name */
+    readonly tpa_name: string;
+    /** Integration type (consider using enum instead) */
+    readonly type: 'accounting' | 'hrms' | 'travel';
+    /** Whether the integration is currently active */
+    is_active: boolean;
+    /** Whether the integration is in beta testing phase */
+    readonly is_beta: boolean;
 }
src/app/integrations/landing-v2/landing-v2.component.ts (2)

80-91: Consider using constants for TPA names.

While the mapping is well-structured, consider defining the TPA names as constants to prevent typos and make maintenance easier. This would also help with type safety and refactoring.

private static readonly TPA_NAMES = {
  NETSUITE: 'Fyle Netsuite Integration',
  SAGE_INTACCT: 'Fyle Sage Intacct Integration',
  // ... other names
} as const;

private readonly tpaNameToIntegrationKeyMap: Record<string, IntegrationAppKey> = {
  [TPA_NAMES.NETSUITE]: 'NETSUITE',
  [TPA_NAMES.SAGE_INTACCT]: 'INTACCT',
  // ... other mappings
};

168-170: Add null safety to prevent undefined returns.

The method could return undefined if connectedApps is not initialized. Consider providing a default value.

-  isAppConnected(appKey: IntegrationAppKey) {
-    return this.connectedApps?.includes(appKey);
+  isAppConnected(appKey: IntegrationAppKey): boolean {
+    return this.connectedApps?.includes(appKey) ?? false;
   }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between dd8b2e5 and 84ceb24.

⛔ Files ignored due to path filters (3)
  • src/assets/logos/intacct-logo-new.png is excluded by !**/*.png
  • src/assets/logos/netsuite-logo-new.png is excluded by !**/*.png
  • src/assets/logos/xero-logo-new.png is excluded by !**/*.png
📒 Files selected for processing (8)
  • src/app/core/models/enum/enum.model.ts (1 hunks)
  • 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)
  • src/app/shared/components/core/badge/badge.component.scss (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/app/integrations/landing-v2/landing-v2.component.html
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: unit-test
🔇 Additional comments (5)
src/app/core/models/enum/enum.model.ts (1)

905-906: LGTM! The SUCCESS theme option is properly integrated.

The addition of SUCCESS to ThemeOption enum follows the existing pattern and correctly corresponds to the new theme-success CSS class.

src/app/shared/components/core/badge/badge.component.scss (1)

25-29: LGTM! The success theme styling is well-implemented.

The theme-success class follows the established pattern and uses consistent Tailwind CSS utility classes to create a cohesive success state visual.

src/app/integrations/landing-v2/landing-v2.component.ts (3)

18-18: LGTM!

The import statement follows Angular's conventions and is properly placed with other service imports.


35-38: LGTM!

The property declarations follow TypeScript best practices with proper typing and immutability where needed.


121-122: LGTM!

The dependency injection follows Angular's best practices.

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();
});
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Enable tests and add coverage for getIntegrations method

The test suite has the following issues:

  1. Tests are disabled using xdescribe
  2. Missing test coverage for getIntegrations method
  3. Dependencies (ApiService, HelperService) are not properly mocked

Here's how to fix these issues:

-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: [
+        { 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 call API', () => {
+      const mockIntegrations = [{ id: 1, org_id: '123', org_name: 'Test Org', tpa_id: '456', tpa_name: 'Test TPA', type: 'test', is_active: true, is_beta: false }];
+      apiService.get.and.returnValue(of(mockIntegrations));
+
+      service.getIntegrations().subscribe(result => {
+        expect(result).toEqual(mockIntegrations);
+        expect(helperService.setBaseApiURL).toHaveBeenCalledWith(AppUrl.INTEGRATION);
+        expect(apiService.get).toHaveBeenCalledWith('/integrations/', {});
+      });
+    });
+  });
 });

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/`, {});
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add error handling and type safety to API call

The getIntegrations method needs:

  1. Error handling for failed API requests
  2. Type safety for API response
  3. Retry logic for transient failures

Here's how to improve the implementation:

-  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(3),
+      catchError(error => {
+        console.error('Failed to fetch integrations:', error);
+        return throwError(() => new Error('Failed to fetch integrations. Please try again later.'));
+      })
+    );
   }

Don't forget to add these imports:

import { Observable, throwError } from 'rxjs';
import { retry, catchError } from 'rxjs/operators';

@@ -14,7 +14,7 @@ const routes: Routes = [
component: LandingComponent
},
{
path: 'landing-v2',
path: 'landing_v2',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Maintain consistent route naming convention

The route path landing_v2 uses underscores while other routes like business-central use hyphens. This inconsistency could lead to confusion and maintenance issues.

Consider using hyphens consistently across all routes:

-        path: 'landing_v2',
+        path: 'landing-v2',

Also consider updating these routes for consistency:

  • bamboo_hrbamboo-hr
  • business_centralbusiness-central
  • qbd_directqbd-direct
📝 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
path: 'landing_v2',
path: 'landing-v2',

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;
});
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add error handling and proper subscription management.

The current implementation has several areas for improvement:

  1. Missing error handling for the HTTP request
  2. No unsubscription from the observable
  3. No loading state management
  4. Lacks type safety for the integration response

Consider implementing it this way:

private subscription = new Subscription();
loading = false;

private storeConnectedApps(): void {
  this.loading = true;
  this.subscription.add(
    this.integrationService.getIntegrations().pipe(
      finalize(() => this.loading = false)
    ).subscribe({
      next: (integrations: Integration[]) => {
        const tpaNames = integrations.map(integration => integration.tpa_name);
        this.connectedApps = tpaNames
          .map(tpaName => this.tpaNameToIntegrationKeyMap[tpaName])
          .filter((key): key is IntegrationAppKey => key !== undefined);
      },
      error: (error) => {
        console.error('Failed to fetch integrations:', error);
        // Consider adding error handling service/toaster notification
      }
    })
  );
}

ngOnDestroy(): void {
  this.subscription.unsubscribe();
}

Also applies to: 252-252

@JustARatherRidiculouslyLongUsername JustARatherRidiculouslyLongUsername merged commit 6eaa285 into new-grid-and-tile-layout Jan 17, 2025
5 checks passed
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
Labels
size/M Medium PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants