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

fix: qbd direct import settings onboarding changes and content changes #1083

Conversation

DhaaraniCIT
Copy link
Contributor

@DhaaraniCIT DhaaraniCIT commented Nov 22, 2024

Description

fix: qbd direct import settings onboarding changes and content changes

Clickup

https://app.clickup.com/t/86cwzceku
Screenshot 2024-11-22 at 4 43 17 PM

Summary by CodeRabbit

Release Notes

  • New Features

    • Enhanced clarity in integration settings for QuickBooks, NetSuite, Xero, and Sage Intacct with updated labels and descriptions.
    • Introduced dynamic components for onboarding import settings and advanced settings, improving layout and user interaction.
    • Added navigation functionality to allow users to return to previous steps in the onboarding process.
  • Bug Fixes

    • Revised sub-labels for vendor imports to clarify terminology and functionality.
  • Documentation

    • Improved user interface instructions for better understanding of integration processes.
  • Chores

    • Added QbdDirectImportSettingsComponent to the shared module for broader accessibility within the application.

Copy link
Contributor

coderabbitai bot commented Nov 22, 2024

Walkthrough

The changes in this pull request primarily focus on enhancing the user interface and functionality related to the onboarding and import settings for integrations with QuickBooks, Xero, and Sage Intacct. Key modifications include updates to text labels for clarity, restructuring HTML components for better layout, and implementing Angular lifecycle practices in the QbdDirectOnboardingImportSettingsComponent. Additionally, the QbdDirectImportSettingsComponent has been added to the shared module, allowing for its use across the application.

Changes

File Change Summary
src/app/branding/fyle-contents-config.ts Updated labels and sub-labels for integration settings, enhancing clarity and specificity.
src/app/integrations/qbd-direct/qbd-direct-onboarding/...html Replaced paragraph element with a structured div container containing dynamic components.
src/app/integrations/qbd-direct/qbd-direct-onboarding/...ts Updated component to be standalone, added properties for branding content and onboarding steps.
src/app/integrations/qbd-direct/qbd-direct-shared/...html Modified subLabel property to clarify vendor import as merchants in Fyle.
src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-shared.module.ts Added QbdDirectImportSettingsComponent to the module imports and exports.
src/app/core/models/common/advanced-settings.model.ts Renamed variable expence_key to expense_key in AdvancedSettingsModel.
src/app/core/models/qbd-direct/qbd-direct-configuration/...ts Updated memo options and payload construction in QbdDirectAdvancedSettingsModel.
src/app/integrations/qbd-direct/qbd-direct-onboarding/...html Replaced paragraph with a structured layout for onboarding steps and advanced settings.
src/app/integrations/qbd-direct/qbd-direct-onboarding/...ts Updated component to be standalone, added properties for branding content and onboarding steps.
src/app/integrations/qbd-direct/qbd-direct-shared/...html Added dynamic content and navigation functionality in the advanced settings footer.
src/app/integrations/qbd-direct/qbd-direct-shared/...ts Introduced navigateToPreviousStep method for user navigation within onboarding workflow.

Possibly related PRs

Suggested labels

size/M

Suggested reviewers

  • ashwin1111

🐰 "In the fields where bunnies play,
Changes sprout like flowers in May.
With clearer paths and steps to take,
We'll hop along, make no mistake!
For every label, every line,
Enhancements bloom, oh how they shine!" 🌼


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?

❤️ Share
🪧 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 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.

@github-actions github-actions bot added the size/S Small PR label Nov 22, 2024
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: 3

🧹 Outside diff range and nitpick comments (5)
src/app/integrations/qbd-direct/qbd-direct-onboarding/qbd-direct-onboarding-import-settings/qbd-direct-onboarding-import-settings.component.html (1)

1-8: Consider simplifying the nested structure.

The current nested div structure could be simplified since each inner div only wraps a single component.

Consider this simplified structure:

<div class="tw-pb-48-px">
-    <div>
        <app-onboarding-steppers [onboardingSteps]="onboardingSteps"></app-onboarding-steppers>
-    </div>
-    <div>
        <app-qbd-direct-import-settings></app-qbd-direct-import-settings>
-    </div>
</div>
src/app/integrations/qbd-direct/qbd-direct-onboarding/qbd-direct-onboarding-import-settings/qbd-direct-onboarding-import-settings.component.ts (2)

27-28: Empty lifecycle hook implementation

The ngOnInit method is currently empty. If no initialization logic is needed, consider removing the OnInit interface implementation.


19-19: Consider providing branding configuration through a service

Direct usage of static brandingContent makes the component tightly coupled and harder to test. Consider injecting this configuration through a service for better maintainability and testability.

Example approach:

// branding.service.ts
@Injectable({
  providedIn: 'root'
})
export class BrandingService {
  getImportSettingConfig() {
    return brandingContent.configuration.importSetting;
  }
}

// component
constructor(
  private workspaceService: WorkspaceService,
  private brandingService: BrandingService
) {
  this.brandingContent = this.brandingService.getImportSettingConfig();
}
src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-import-settings/qbd-direct-import-settings.component.html (1)

Line range hint 1-99: Consider architectural improvements for better maintainability.

While the current implementation is functional, consider the following improvements:

  1. Extract hardcoded text strings (like dropdown placeholders) to localization files for better maintainability and future internationalization support.
  2. Break down the large form template into smaller, reusable components to improve readability and maintainability.
src/app/branding/fyle-contents-config.ts (1)

69-70: LGTM! The text changes improve clarity.

The updated labels better describe the mapping relationships and provide clearer context about the functionality. The changes align well with the PR objective of improving the onboarding experience.

Consider standardizing the casing in the labels for consistency:

-                importCategoriesLabel: 'Import Chart of Accounts as Categories in ' + brandingConfig.brandName,
+                importCategoriesLabel: 'Import chart of accounts as categories in ' + brandingConfig.brandName,

Also applies to: 73-73

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 8ea1887 and 9528c3b.

📒 Files selected for processing (5)
  • src/app/branding/fyle-contents-config.ts (1 hunks)
  • src/app/integrations/qbd-direct/qbd-direct-onboarding/qbd-direct-onboarding-import-settings/qbd-direct-onboarding-import-settings.component.html (1 hunks)
  • src/app/integrations/qbd-direct/qbd-direct-onboarding/qbd-direct-onboarding-import-settings/qbd-direct-onboarding-import-settings.component.ts (1 hunks)
  • src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-import-settings/qbd-direct-import-settings.component.html (1 hunks)
  • src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-shared.module.ts (2 hunks)
🔇 Additional comments (5)
src/app/integrations/qbd-direct/qbd-direct-onboarding/qbd-direct-onboarding-import-settings/qbd-direct-onboarding-import-settings.component.html (2)

6-6: Verify component registration.

Ensure that app-qbd-direct-import-settings component is properly registered in the module.

✅ Verification successful

Component is properly registered as a standalone component

The component is correctly registered as a standalone component with the necessary imports (CommonModule, SharedModule, MultiSelectModule) in its decorator. No module registration is required as it uses Angular's standalone component feature.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify component registration and existence

# Check if the component is declared in a module
ast-grep --pattern 'declarations: [
  $$$
  QbdDirectImportSettingsComponent,
  $$$
]'

# Check if the component exists
fd "qbd-direct-import-settings.component.ts"

Length of output: 250


Script:

#!/bin/bash
# Check if the component is declared in any module file
rg "QbdDirectImportSettingsComponent" -A 2 -B 2 "*.module.ts"

# Check the component's module imports
cat "src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-import-settings/qbd-direct-import-settings.component.ts"

Length of output: 12157


3-3: Verify onboardingSteps initialization.

Ensure that the onboardingSteps property is properly initialized in the component class to prevent potential undefined errors.

✅ Verification successful

The onboardingSteps property is properly initialized

The code shows that the onboardingSteps property in the QbdDirectOnboardingImportSettingsComponent is properly initialized during class instantiation:

onboardingSteps: OnboardingStepper[] = new QbdDirectOnboardingModel().getOnboardingSteps(this.brandingContent.stepName, this.workspaceService.getOnboardingState());

The initialization:

  1. Creates a new instance of QbdDirectOnboardingModel
  2. Calls getOnboardingSteps() with required parameters
  3. Assigns the returned array to the property

This is a safe implementation that prevents undefined errors as the property is initialized immediately when the component is created.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if onboardingSteps is properly initialized in the component

# Look for onboardingSteps initialization in the component
ast-grep --pattern 'class QbdDirectOnboardingImportSettingsComponent {
  $$$
  onboardingSteps = $_;
  $$$
}'

# Also check if it's initialized in ngOnInit
ast-grep --pattern 'ngOnInit() {
  $$$
  this.onboardingSteps = $_;
  $$$
}'

Length of output: 190


Script:

#!/bin/bash
# Let's check the component file directly to see the initialization
fd "qbd-direct-onboarding-import-settings.component.ts" --exec cat {}

# Also search for any references to onboardingSteps in the file
rg "onboardingSteps" -A 3 -B 3

Length of output: 100601

src/app/integrations/qbd-direct/qbd-direct-onboarding/qbd-direct-onboarding-import-settings/qbd-direct-onboarding-import-settings.component.ts (1)

1-16: LGTM! Component setup follows Angular best practices

The component is properly configured as standalone with appropriate module imports.

src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-import-settings/qbd-direct-import-settings.component.html (2)

68-68: LGTM! Clear and descriptive subLabel text.

The updated subLabel text provides a clearer explanation of how vendors from QuickBooks are imported as merchants in Fyle, making it more user-friendly and explicit about the data transformation that occurs.


Line range hint 66-72: Verify feature flag configuration.

The vendors-as-merchants section is conditionally rendered based on two flags: isImportMerchantsAllowed and brandingFeatureConfig.featureFlags.importSettings.importVendorsAsMerchants. Let's verify these flags are properly configured.

✅ Verification successful

Let me gather more information about the isImportMerchantsAllowed property since the previous search didn't find its definition.


Let me check the component implementation and template to verify the complete feature flag setup.


Feature flags for vendors-as-merchants import are properly configured

The verification shows that the feature flags are correctly set up:

  1. The isImportMerchantsAllowed property is initialized as true by default and is toggled based on the auto_create_merchant_as_vendor setting from advanced settings.
  2. The template correctly uses both required flags:
    • isImportMerchantsAllowed
    • brandingFeatureConfig.featureFlags.importSettings.importVendorsAsMerchants
  3. The section is properly rendered with the correct conditional logic: *ngIf="isImportMerchantsAllowed && brandingFeatureConfig.featureFlags.importSettings.importVendorsAsMerchants"
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check feature flag configuration in the codebase

# Search for feature flag definitions and usage
rg -A 3 "importVendorsAsMerchants" --type ts

# Search for isImportMerchantsAllowed property
ast-grep --pattern 'isImportMerchantsAllowed = $_'

Length of output: 12866


Script:

#!/bin/bash
# Search for isImportMerchantsAllowed in the codebase
rg "isImportMerchantsAllowed" --type ts -A 3

# Search for the component where the feature is used
rg -l "qbd-direct-import-settings.component"

Length of output: 5159


Script:

#!/bin/bash
# Get the component implementation
cat src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-import-settings/qbd-direct-import-settings.component.ts

# Get the component template
cat src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-import-settings/qbd-direct-import-settings.component.html

Length of output: 19448

@@ -5,6 +5,7 @@ import { QbdDirectDownloadFileComponent } from './qbd-direct-download-file/qbd-d
import { QbdDirectSetupConnectionComponent } from './qbd-direct-setup-connection/qbd-direct-setup-connection.component';
import { QbdDirectDataSyncComponent } from './qbd-direct-data-sync/qbd-direct-data-sync.component';
import { QbdDirectExportSettingsComponent } from './qbd-direct-export-settings/qbd-direct-export-settings.component';
import { QbdDirectImportSettingsComponent } from './qbd-direct-import-settings/qbd-direct-import-settings.component';
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Component exists and follows Angular conventions

The QbdDirectImportSettingsComponent exists at the correct relative path and follows proper Angular component structure with appropriate decorators, selector, and metadata. However, I notice it's marked as standalone: true, which means it shouldn't be declared in NgModules. The shared module should use imports array instead of both imports and declarations.

🔗 Analysis chain

Verify the imported component exists

The import statement follows Angular conventions, but let's verify the component exists at the specified path.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify QbdDirectImportSettingsComponent exists and follows Angular component structure

# Check if the component file exists
fd "qbd-direct-import-settings.component.ts" 

# Verify component structure
ast-grep --pattern 'class QbdDirectImportSettingsComponent {
  $$$
}'

Length of output: 233


Script:

#!/bin/bash
# Let's verify the component's structure and decorators
rg -A 5 "@Component" src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-import-settings/qbd-direct-import-settings.component.ts

Length of output: 396

Comment on lines +18 to +19
QbdDirectExportSettingsComponent,
QbdDirectImportSettingsComponent
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Components are incorrectly configured in the shared module

The verification revealed that the components are incorrectly placed in the imports array instead of the declarations array of the shared module. Components should be declared in declarations array and then exported via exports array. The current configuration won't work as expected since Angular components cannot be imported directly.

  • Move QbdDirectExportSettingsComponent and QbdDirectImportSettingsComponent from imports to declarations array in src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-shared.module.ts
  • The same applies to other components currently in the imports array: QbdDirectDownloadFileComponent, QbdDirectSetupConnectionComponent, and QbdDirectDataSyncComponent
🔗 Analysis chain

LGTM! Module configuration follows best practices

The changes correctly:

  • Add the component to both imports and exports arrays
  • Follow the shared module pattern for component reusability
  • Maintain consistency with existing component configuration

Also applies to: 25-26


Let me verify the module configuration by checking the actual declarations in the routing and shared modules.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the component usage across the application

# Find where the shared module is imported
rg -l "QbdDirectSharedModule" 

# Find direct usage of the component
rg -l "QbdDirectImportSettingsComponent"

Length of output: 855


Script:

#!/bin/bash
# Check the component declarations in shared module
rg -A 10 "declarations:" src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-shared.module.ts

# Check the component usage in routing module
rg -A 10 "const routes" src/app/integrations/qbd-direct/qbd-direct-main/qbd-direct-configuration/qbd-direct-configuration-routing.module.ts

# Check if component is used in templates
rg "<qbd-direct-import-settings" 
rg "<qbd-direct-export-settings"

Length of output: 833

Comment on lines +19 to +21
brandingContent = brandingContent.configuration.importSetting;

onboardingSteps: OnboardingStepper[] = new QbdDirectOnboardingModel().getOnboardingSteps(this.brandingContent.stepName, this.workspaceService.getOnboardingState());
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

Move service-dependent initialization to ngOnInit

The initialization of onboardingSteps in the property declaration is using an injected service, which is an anti-pattern in Angular. This could cause issues during testing and potential race conditions.

Move the initialization to ngOnInit:

-  onboardingSteps: OnboardingStepper[] = new QbdDirectOnboardingModel().getOnboardingSteps(this.brandingContent.stepName, this.workspaceService.getOnboardingState());
+  onboardingSteps: OnboardingStepper[];
+
+  ngOnInit(): void {
+    this.onboardingSteps = new QbdDirectOnboardingModel().getOnboardingSteps(
+      this.brandingContent.stepName,
+      this.workspaceService.getOnboardingState()
+    );
+  }
📝 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
brandingContent = brandingContent.configuration.importSetting;
onboardingSteps: OnboardingStepper[] = new QbdDirectOnboardingModel().getOnboardingSteps(this.brandingContent.stepName, this.workspaceService.getOnboardingState());
brandingContent = brandingContent.configuration.importSetting;
onboardingSteps: OnboardingStepper[];
ngOnInit(): void {
this.onboardingSteps = new QbdDirectOnboardingModel().getOnboardingSteps(
this.brandingContent.stepName,
this.workspaceService.getOnboardingState()
);
}

@github-actions github-actions bot added size/M Medium PR and removed size/S Small PR labels Nov 25, 2024
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 and nitpick comments (8)
src/app/integrations/qbd-direct/qbd-direct-onboarding/qbd-direct-onboarding-advanced-settings/qbd-direct-onboarding-advanced-settings.component.ts (3)

13-17: Update styleUrl to styleUrls

The styleUrl property in the component decorator should be styleUrls (plural) as per Angular's component metadata interface. While styleUrl might work now, it's recommended to use the standard property name for better maintainability.

@Component({
  selector: 'app-qbd-direct-onboarding-advanced-settings',
  standalone: true,
  imports: [CommonModule, QbdDirectSharedModule, SharedModule, QbdDirectAdvancedSettingsComponent],
  templateUrl: './qbd-direct-onboarding-advanced-settings.component.html',
-  styleUrl: './qbd-direct-onboarding-advanced-settings.component.scss'
+  styleUrls: ['./qbd-direct-onboarding-advanced-settings.component.scss']
})

28-29: Remove empty lifecycle hook

The ngOnInit method is currently empty. If no initialization logic is needed, you can remove the empty method and the OnInit interface implementation.


22-22: Move complex initialization to ngOnInit

The initialization of onboardingSteps involves multiple operations and service calls. It's recommended to move this logic to the ngOnInit lifecycle hook for better clarity and control over the initialization process.

-  onboardingSteps: OnboardingStepper[] = new QbdDirectOnboardingModel().getOnboardingSteps(this.brandingContent.stepName, this.workspaceService.getOnboardingState());
+  onboardingSteps: OnboardingStepper[];
+
+  ngOnInit(): void {
+    const onboardingState = this.workspaceService.getOnboardingState();
+    this.onboardingSteps = new QbdDirectOnboardingModel().getOnboardingSteps(
+      this.brandingContent.stepName,
+      onboardingState
+    );
+  }
src/app/core/models/qbd-direct/qbd-direct-configuration/qbd-direct-advanced-settings.model.ts (1)

29-29: The display value approach is good but consider centralizing the transformation logic.

The approach of using user-friendly display values in the UI while maintaining technical keys in the backend is good. However, consider:

  1. Creating constants for both values to avoid string literals
  2. Centralizing the transformation logic in a utility method
  3. Adding this pattern to the documentation for consistency across the codebase
// Suggested utility method:
static transformDisplayToTechnicalKey(value: string): string {
    const displayToTechnicalMap: Record<string, string> = {
        'Expense/Report ID': 'expense_key'
        // Add more mappings as needed
    };
    return displayToTechnicalMap[value] || value;
}

Also applies to: 61-63

src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-advanced-settings/qbd-direct-advanced-settings.component.html (2)

57-59: Move label construction to component class for better maintainability.

The current string concatenation in the template makes the code harder to maintain and less reusable. Consider moving this logic to the component class.

Example refactor:

// In component class
get reimbursableEntityLabel(): string {
  return `${this.brandingContent.autoCreateReimbursableEnitityLabel}${this.employeeMapping | snakeCaseToSpaceCase | lowercase}s in QuickBooks Desktop`;
}

get reimbursableEntitySubLabel(): string {
  return `${this.brandingContent.autoCreateReimbursableEnititySubLabel}${this.employeeMapping | snakeCaseToSpaceCase | lowercase} record for them, ensuring expenses are posted to this new record.`;
}
-[label]="brandingContent.autoCreateReimbursableEnitityLabel + (employeeMapping | snakeCaseToSpaceCase | lowercase) + 's in QuickBooks Desktop'"
-[subLabel]="brandingContent.autoCreateReimbursableEnititySubLabel + (employeeMapping | snakeCaseToSpaceCase | lowercase) + ' record for them, ensuring expenses are posted to this new record.'"
+[label]="reimbursableEntityLabel"
+[subLabel]="reimbursableEntitySubLabel"

131-131: Improve template readability by breaking down long line.

The configuration step footer contains multiple bindings on a single line, making it difficult to read and maintain.

Consider breaking it down for better readability:

-<app-configuration-step-footer [ctaText]="!saveInProgress ? (isOnboarding ? ConfigurationCtaText.SAVE_AND_CONTINUE : ConfigurationCtaText.SAVE) : ConfigurationCtaText.SAVING" [isButtonDisabled]="!advancedSettingsForm.valid || saveInProgress || !skipExportForm.valid" (save)="save()" [showBackButton]="true" (navigateToPreviousStep)="navigateToPreviousStep()"></app-configuration-step-footer>
+<app-configuration-step-footer
+    [ctaText]="!saveInProgress ? (isOnboarding ? ConfigurationCtaText.SAVE_AND_CONTINUE : ConfigurationCtaText.SAVE) : ConfigurationCtaText.SAVING"
+    [isButtonDisabled]="!advancedSettingsForm.valid || saveInProgress || !skipExportForm.valid"
+    [showBackButton]="true"
+    (save)="save()"
+    (navigateToPreviousStep)="navigateToPreviousStep()">
+</app-configuration-step-footer>

Additionally, consider moving the complex ctaText logic to the component class:

get ctaText(): string {
  if (this.saveInProgress) {
    return ConfigurationCtaText.SAVING;
  }
  return this.isOnboarding ? ConfigurationCtaText.SAVE_AND_CONTINUE : ConfigurationCtaText.SAVE;
}
src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-advanced-settings/qbd-direct-advanced-settings.component.ts (2)

140-142: Consider enhancing navigation implementation

A few suggestions to improve the navigation implementation:

  1. Define routes as constants to prevent typos and enable easier maintenance
  2. Consider preserving form state during navigation
  3. Add type safety for the navigation path
+const QBD_ROUTES = {
+  IMPORT_SETTINGS: '/integrations/qbd_direct/onboarding/import_settings'
+} as const;

 navigateToPreviousStep() {
-  this.router.navigate([`/integrations/qbd_direct/onboarding/import_settings`]);
+  // Preserve form state if needed
+  const navigationExtras: NavigationExtras = {
+    state: {
+      formData: this.advancedSettingsForm.value
+    }
+  };
+  this.router.navigate([QBD_ROUTES.IMPORT_SETTINGS], navigationExtras);
 }

Line range hint 40-40: Fix type safety: Change 'any' type to boolean

The isOnboarding property should be explicitly typed as boolean instead of any.

-isOnboarding: any;
+isOnboarding: boolean;
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 9528c3b and d059b71.

📒 Files selected for processing (7)
  • src/app/branding/fyle-contents-config.ts (1 hunks)
  • src/app/core/models/common/advanced-settings.model.ts (1 hunks)
  • src/app/core/models/qbd-direct/qbd-direct-configuration/qbd-direct-advanced-settings.model.ts (2 hunks)
  • src/app/integrations/qbd-direct/qbd-direct-onboarding/qbd-direct-onboarding-advanced-settings/qbd-direct-onboarding-advanced-settings.component.html (1 hunks)
  • src/app/integrations/qbd-direct/qbd-direct-onboarding/qbd-direct-onboarding-advanced-settings/qbd-direct-onboarding-advanced-settings.component.ts (1 hunks)
  • src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-advanced-settings/qbd-direct-advanced-settings.component.html (2 hunks)
  • src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-advanced-settings/qbd-direct-advanced-settings.component.ts (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • src/app/core/models/common/advanced-settings.model.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/app/branding/fyle-contents-config.ts
🔇 Additional comments (8)
src/app/integrations/qbd-direct/qbd-direct-onboarding/qbd-direct-onboarding-advanced-settings/qbd-direct-onboarding-advanced-settings.component.html (2)

1-4: LGTM! Clean and well-structured template.

The template follows a clear hierarchical structure with proper component composition.

Let's verify the integration with related components:

✅ Verification successful

Template structure and component integration verified

The verification shows consistent patterns across the codebase:

  • The tw-pb-48-px class is consistently used across all onboarding components for bottom padding
  • The app-onboarding-steppers component with [onboardingSteps] input is used uniformly across all integration onboarding flows
  • The template structure follows the same pattern used in other integration onboarding components (QBO, Xero, NetSuite, etc.)
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the existence and structure of related components

# Check for the onboarding-steppers component
echo "Checking onboarding-steppers component..."
ast-grep --pattern 'class OnboardingSteppersComponent {
  $$$
  @Input() onboardingSteps
  $$$
}'

# Check for the advanced settings component
echo "Checking advanced settings component..."
ast-grep --pattern 'class QbdDirectAdvancedSettingsComponent {
  $$$
}'

# Check for consistent usage of tw-pb-48-px class
echo "Checking consistent padding usage..."
rg "tw-pb-48-px" -A 2 -B 2

Length of output: 19417


2-2: Verify onboardingSteps initialization.

The template binds to onboardingSteps property. Ensure it's properly initialized in the component class.

✅ Verification successful

onboardingSteps is properly initialized

The property is correctly initialized as a class field using the QbdDirectOnboardingModel to generate the steps based on branding content and workspace state:

onboardingSteps: OnboardingStepper[] = new QbdDirectOnboardingModel().getOnboardingSteps(this.brandingContent.stepName, this.workspaceService.getOnboardingState());
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify onboardingSteps initialization in the component

# Check for onboardingSteps initialization
ast-grep --pattern 'class QbdDirectOnboardingAdvancedSettingsComponent {
  $$$
  onboardingSteps = $_;
  $$$
}'

Length of output: 113


Script:

#!/bin/bash
# Let's try a different approach to find the component file and check its content
fd -e ts "qbd-direct-onboarding-advanced-settings.component" --exec cat {}

Length of output: 2395

src/app/integrations/qbd-direct/qbd-direct-onboarding/qbd-direct-onboarding-advanced-settings/qbd-direct-onboarding-advanced-settings.component.ts (3)

1-9: LGTM! Well-organized imports

The imports are properly structured and include all necessary dependencies for the component's functionality.


24-26: LGTM! Proper dependency injection

The WorkspaceService is correctly injected through the constructor.


13-13: Verify component usage and integration

Since this is a standalone component, let's verify its integration within the application.

✅ Verification successful

Component integration is properly configured

The component is correctly integrated as a standalone component and is properly used in the application:

  • It's registered in the routing configuration at path 'advanced_settings'
  • It's part of the QBD Direct onboarding flow as a child route
  • The component is used in the template as <app-qbd-direct-advanced-settings> within the onboarding flow
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check how this component is imported and used in other parts of the application

# Find files importing this component
rg -l "QbdDirectOnboardingAdvancedSettingsComponent" --type ts

# Find template files using this component's selector
rg -l "app-qbd-direct-onboarding-advanced-settings" --type html

Length of output: 519


Script:

#!/bin/bash
# Let's check the routing configuration to understand how this standalone component is integrated
cat src/app/integrations/qbd-direct/qbd-direct-onboarding/qbd-direct-onboarding-routing.module.ts

# Also check if there are any template files in the parent directories that might use this component
fd . src/app/integrations/qbd-direct/qbd-direct-onboarding/ -e html -x cat {}

Length of output: 9925

src/app/core/models/qbd-direct/qbd-direct-configuration/qbd-direct-advanced-settings.model.ts (1)

29-29: Verify the consistent handling of "Expense/Report ID" display value.

The change from "expense_key" to "Expense/Report ID" makes the option more user-friendly. However, ensure this display value is consistently handled across the UI and properly transformed back to "expense_key" when saving to the backend.

✅ Verification successful

Let me analyze the code further by checking the implementation of the transformation logic.


Let me try a different pattern to find the relevant code.


The display value transformation is properly handled

The code shows proper handling of the display value transformation:

  1. defaultTopMemoOptions() returns the user-friendly display value "Expense/Report ID"
  2. In constructPayload(), before sending to backend, the display value is correctly transformed back to the technical key:
    const index = topMemo.indexOf('Expense/Report ID');
    topMemo[index] = 'expense_key';
  3. The technical key expense_key is consistently used in defaultMemoFields() which defines the backend-compatible values.
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for other occurrences of both the technical key and display value
# to ensure consistent handling

echo "Searching for technical key usage..."
rg "expense_key" --type ts

echo "Searching for display value usage..."
rg "Expense/Report ID" --type ts

Length of output: 981


Script:

#!/bin/bash
# Check the implementation of constructPayload() method where the transformation happens
ast-grep --pattern 'constructPayload() {
  $$$
}'

# Also check for any other methods that might handle this transformation
ast-grep --pattern 'transformMemo($$$) {
  $$$
}'

Length of output: 103


Script:

#!/bin/bash
# Get more context around the transformation by showing the surrounding lines
rg -B 5 -A 10 "Expense/Report ID" --type ts

# Also check for any class methods in the file
ast-grep --pattern 'class QbdDirectAdvancedSettingsModel {
  $$$
}'

Length of output: 4579

src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-advanced-settings/qbd-direct-advanced-settings.component.html (1)

131-131: Verify navigation flow in the onboarding process.

The addition of back navigation needs verification to ensure proper routing in the onboarding flow.

✅ Verification successful

Based on the results from our verification attempts, I can now generate the final response:

Back navigation is properly implemented and follows the standard pattern

The navigation implementation follows the established pattern seen across the codebase:

  • The component uses the shared configuration-step-footer component which handles back button clicks
  • The navigateToPreviousStep() method is implemented to route to the import settings page
  • The routing path follows the standard /integrations/qbd_direct/onboarding/import_settings pattern
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the navigation flow implementation

# Check for route definitions
ast-grep --pattern 'const routes: Routes = [
  $$$
  { path: $_, component: QbdDirectAdvancedSettingsComponent }
  $$$
]'

# Check for navigation method implementation
ast-grep --pattern 'navigateToPreviousStep() {
  $$$
}'

Length of output: 181


Script:

#!/bin/bash
# Let's try a different approach to verify the navigation implementation

# Search for the component class implementation
rg -l "QbdDirectAdvancedSettingsComponent" --type ts

# Search for navigation-related code in the component
rg "navigateToPreviousStep" -A 5 --type ts

# Look for router imports and usage
rg "Router|ActivatedRoute" --type ts -A 2 -C 2 "qbd-direct-advanced-settings.component.ts"

Length of output: 27344


Script:

#!/bin/bash
# Let's check the specific QBD Direct advanced settings component implementation and routing

# Check the component implementation
rg -A 10 "class QbdDirectAdvancedSettingsComponent" --type ts

# Check the routing configuration
rg -A 5 "path.*qbd_direct.*advanced_settings" --type ts

# Check router import and constructor
rg -A 10 "constructor.*private.*router.*Router" "qbd-direct-advanced-settings.component.ts"

Length of output: 1854

src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-advanced-settings/qbd-direct-advanced-settings.component.ts (1)

140-142: Verify navigation path consistency

The navigation path looks correct but let's ensure it's consistent with the application's routing configuration.

✅ Verification successful

Navigation path is consistent with the application flow

The navigation path /integrations/qbd_direct/onboarding/import_settings is correctly implemented and aligns with the application's routing structure and onboarding flow. The evidence shows:

  • The path is part of a well-defined onboarding state sequence in QbdDirectComponent
  • It's consistently used across components in the correct order:
    • Export Settings -> Import Settings -> Advanced Settings
  • The navigation is properly handled through the Angular Router
  • The path matches the lazy-loaded routing structure defined in QbdDirectRoutingModule
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the routing path exists and is consistent

# Search for route definitions containing 'qbd_direct/onboarding/import_settings'
rg -l "qbd_direct/onboarding/import_settings" --type ts

# Search for other usages of this route to verify consistency
rg "navigate.*qbd_direct/onboarding/import_settings" --type ts

# Look for route definitions in routing modules
ast-grep --pattern 'const routes: Routes = [
  $$$
  { path: $_, $$ }
  $$$
]'

Length of output: 905


Script:

#!/bin/bash
# Let's check the routing module content specifically for QBD Direct
rg -A 5 "path.*qbd_direct/onboarding/import_settings" --type ts

# Also check the main QBD Direct component to understand the routing structure
rg "const\s+routes.*=.*\[" -A 20 src/app/integrations/qbd-direct/qbd-direct.component.ts

# Check if there are any other components that might be using this route
ast-grep --pattern 'class $_ implements OnInit {
  $$$
  constructor($$$) {
    $$$
  }
  $$$
}'  src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-import-settings/qbd-direct-import-settings.component.ts

Length of output: 40226


Script:

#!/bin/bash
# Let's check the routing module and component structure
rg -l "export.*QbdDirectRoutingModule" --type ts

# Check the actual routing configuration
rg "const\s+routes.*=.*\[" -A 30 src/app/integrations/qbd-direct/qbd-direct-routing.module.ts

# Check for any other components that might be using these routes
rg "qbd_direct/onboarding" -A 2 -B 2 --type ts

Length of output: 10472

Comment on lines +61 to +63
const index = topMemo.indexOf('Expense/Report ID');
topMemo[index] = 'expense_key';

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 safety checks for the memo field transformation.

The current implementation could be more robust. Consider these improvements:

-        const index = topMemo.indexOf('Expense/Report ID');
-        topMemo[index] = 'expense_key';
+        if (topMemo) {
+            const index = topMemo.indexOf('Expense/Report ID');
+            if (index !== -1) {
+                // Create a new array instead of mutating the original
+                topMemo = [...topMemo];
+                topMemo[index] = 'expense_key';
+            }
+        }

This change:

  1. Adds null check for topMemo
  2. Verifies the element exists before replacement
  3. Avoids direct array mutation
  4. Makes the code more defensive against potential runtime errors

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

Copy link

Unit Test Coverage % values
Statements 34.85% ( 4247 / 12185 )
Branches 28.83% ( 1233 / 4276 )
Functions 27.48% ( 939 / 3417 )
Lines 34.99% ( 4177 / 11935 )

@DhaaraniCIT DhaaraniCIT merged commit 43d4347 into qbd-direct-onboarding-export-settings-main Nov 25, 2024
5 checks passed
DhaaraniCIT added a commit that referenced this pull request Nov 25, 2024
#1082)

* qbd direct export settings onboarding changes and content changes

* fix: qbd direct import settings onboarding changes and content changes (#1083)

* qbd direct import settings onboarding changes and content changes

* qbd direct advanced settings onboarding changes and content changes (#1084)
DhaaraniCIT added a commit that referenced this pull request Nov 25, 2024
* QA fixes

* qa fixes

* qa fixes

* fix: qbd direct export settings onboarding changes and content changes (#1082)

* qbd direct export settings onboarding changes and content changes

* fix: qbd direct import settings onboarding changes and content changes (#1083)

* qbd direct import settings onboarding changes and content changes

* qbd direct advanced settings onboarding changes and content changes (#1084)

* QA fixes

* fix: QBD direct bug fixes (#1085)

* QBD direct bug fixes

* QBD direct bug fixes

* QBD direct bug fixes
DhaaraniCIT added a commit that referenced this pull request Dec 3, 2024
* fead: folder creation

* PR comments fix

* feat: Qbd checkbox button creation (#1054)

* feat: checkbox button creation

* PR comments fix

* feat: onboarding basic setup (#1055)

* feat: onboarding basic setup

* feat: qbd direct onboarding landing page (#1056)

* feat: qbd direct onboarding landing page

* feat: qbd-direct-onboarding-pre-requisite implementation

* PR comments fix

* PR fix

* updateWorkspaceOnboardingState service return type update

* qbd direct logo update

* feat: qbd-direct onboarding prerequisite UI implementation (#1058)

* feat: qbd-direct-onboarding-pre-requisite implementation

* styling changes

* unit test fix

* step footer contentt fix

* pre requisite Ui updation

* PR comments fix

* PR comments fix

* feat: Download qwd file UI changes (#1059)

* feat: Download qwd file UI changes

* download file Ui updation

* download file Ui updation

* download file Ui updation

* download file Ui updation

* feat: qbd connector setup UI changes (#1060)

* feat: qbd connector setup UI changes

* Merge branch qbd-direct-onboarding-download-file-UI into qbd-direct-step-connector-UI

* feat: Qbd direct connection data sync UI changes (#1061)

* feat: Qbd direct connection data sync UI changes

* input made required

* svg update

* feat: qbd direct pre requisite ts changes (#1062)

* feat: qbd direct pre requisite ts changes

* feat: qbd direct pre requisite ts changes

* feat: qbd direct connector download file ts changes (#1063)

* feat: qbd direct connector download file ts changes

* feat: qbd direct connector setup connector ts changes (#1064)

* feat: qbd direct connector setup connector ts changes

* feat: qbd direct connector data sync up ts changes (#1065)

* PR comment fix

* PR comment fix

* Qbd direct connector data sync up ts (#1070)

* feat: qbd direct pre requisite ts changes

* feat: qbd direct pre requisite ts changes

* feat: qbd direct connector download file ts changes

* feat: qbd direct connector setup connector ts changes

* feat: qbd direct connector data sync up ts changes

* feat: QBD direct main connection page business logic (#1066)

* feat: QBD direct main connection page business logic

* onboarding connection ts changes

* onboarding connection ts changes

---------

Co-authored-by: ashwin1111 <[email protected]>

---------

Co-authored-by: ashwin1111 <[email protected]>

---------

Co-authored-by: ashwin1111 <[email protected]>

---------

Co-authored-by: ashwin1111 <[email protected]>

---------

Co-authored-by: ashwin1111 <[email protected]>

---------

Co-authored-by: ashwin1111 <[email protected]>

---------

Co-authored-by: ashwin1111 <[email protected]>

* checkbox button fix

* QA fixes

* qa fixes

* qa fixes

* fix: qbd direct export settings onboarding changes and content changes (#1082)

* qbd direct export settings onboarding changes and content changes

* fix: qbd direct import settings onboarding changes and content changes (#1083)

* qbd direct import settings onboarding changes and content changes

* qbd direct advanced settings onboarding changes and content changes (#1084)

* QA fixes

* QBD direct bug fixes

* QBD direct bug fixes

* JE changes in export settings and mapping

* PR comments fix

---------

Co-authored-by: ashwin1111 <[email protected]>
@coderabbitai coderabbitai bot mentioned this pull request Dec 3, 2024
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