feat: onboarding basic setup #1055
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces enhancements to the configuration and onboarding processes for QuickBooks integration. It adds a Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Warning Rate limit exceeded@github-actions[bot] has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 20 minutes and 39 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (5)
src/app/core/models/qbd-direct/qbd-direct-configuration/qbd-direct-connector.model.ts (1)
5-15: Add trailing comma for better git diffsConsider adding a trailing comma after the last property for consistency and cleaner git diffs when new properties are added.
qwc: string, created_at: Date, - updated_at: Date + updated_at: Date,src/app/integrations/qbd-direct/qbd-direct-onboarding/qbd-direct-onboarding-routing.module.ts (1)
21-24: Standardize route path naming conventionThe path
'pre_requisite'uses snake_case while other routes in the module use camelCase (e.g.,'exportSettings'). Consider standardizing to camelCase for consistency.{ - path: 'pre_requisite', + path: 'preRequisite', component: QbdDirectOnboardingPreRequisiteComponent },src/app/core/models/qbd-direct/qbd-direct-configuration/qbd-direct-onboarding.model.ts (1)
5-18: Consider making the type definition more strictThe type could benefit from readonly properties to prevent accidental modifications to the step numbers during runtime.
Consider this improvement:
-type QbdOnboardingStepperMap = { +type QbdOnboardingStepperMap = Readonly<{ [QbdDirectOnboardingState.YET_TO_START]: number; // ... other states [QbdDirectOnboardingState.COMPLETE]: number; -} +}>src/app/branding/c1-contents-config.ts (1)
13-17: Consider adding similar preRequisite sections to other integrations.For consistency in user experience, consider adding similar preRequisite sections to other integration configurations (NetSuite, Xero, Sage Intacct, QuickBooks Online) if they also have prerequisite steps.
Example for NetSuite integration:
netsuite: { configuration: { + preRequisite: { + stepName: 'Prerequisites', + configurationHeaderText: 'Getting started', + configurationSubHeaderText: 'Begin your NetSuite integration by completing the following steps.' + }, connector: { // existing configuration },src/app/branding/fyle-contents-config.ts (1)
Line range hint
1-1024: Consider refactoring to improve maintainability.The configuration file could benefit from the following improvements:
- Split the configurations into separate files per integration type (QBD, NetSuite, Xero, Intacct).
- Create reusable constants for common text patterns.
- Use template literals instead of string concatenation with brandingConfig.brandName.
Example refactor:
// src/app/branding/constants/common-text.ts export const COMMON_TEXT = { IMPORT_CATEGORIES: (brandName: string) => `Import the Chart of Accounts as Categories in ${brandName}`, IMPORT_CATEGORIES_SUB: (brandName: string) => `Imported account will be available as Categories in ${brandName}.` // ... more common text }; // src/app/branding/configs/qbd-contents.config.ts import { COMMON_TEXT } from '../constants/common-text'; import { brandingConfig } from '../config'; export const qbdContents = { qbd_direct: { // ... QBD specific config } }; // src/app/branding/fyle-contents-config.ts export const fyleContents = { ...qbdContents, ...netsuiteContents, ...xeroContents, ...intacctContents };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
src/app/branding/c1-contents-config.ts(1 hunks)src/app/branding/fyle-contents-config.ts(1 hunks)src/app/core/models/branding/content-configuration.model.ts(1 hunks)src/app/core/models/qbd-direct/qbd-direct-configuration/qbd-direct-connector.model.ts(1 hunks)src/app/core/models/qbd-direct/qbd-direct-configuration/qbd-direct-onboarding.model.ts(1 hunks)src/app/integrations/qbd-direct/qbd-direct-onboarding/qbd-direct-onboarding-routing.module.ts(2 hunks)
🔇 Additional comments (8)
src/app/core/models/qbd-direct/qbd-direct-configuration/qbd-direct-connector.model.ts (2)
1-3: LGTM: QbdConnectorPost type
The POST model is well-defined and follows TypeScript best practices.
8-11: Verify secure handling of sensitive data
Let's verify how sensitive data (username, password, ticket_id) is handled throughout the codebase.
src/app/integrations/qbd-direct/qbd-direct-onboarding/qbd-direct-onboarding-routing.module.ts (1)
10-10: LGTM!
The import statement follows Angular's naming conventions and maintains consistency with other component imports.
src/app/core/models/qbd-direct/qbd-direct-configuration/qbd-direct-onboarding.model.ts (1)
66-79: Prevent potential off-by-one errors
The onboardingStateStepMap uses 1-based indexing while array access uses 0-based indexing. This could lead to confusion and potential errors.
Let's verify the array access logic:
Consider either:
- Using 0-based indexing in the map and removing the -1 in array access
- Adding a comment explaining the indexing difference
private readonly onboardingStateStepMap: QbdOnboardingStepperMap = {
- [QbdDirectOnboardingState.YET_TO_START]: 1,
+ // Using 1-based indexing for step numbers
+ // Subtract 1 when accessing onboardingSteps array
+ [QbdDirectOnboardingState.YET_TO_START]: 1,Also applies to: 90-92
src/app/core/models/branding/content-configuration.model.ts (1)
9-13: LGTM! The preRequisite structure aligns with onboarding requirements.
The addition of the preRequisite section with stepName, configurationHeaderText, and configurationSubHeaderText properties follows the established pattern in the configuration model and supports the onboarding setup feature.
Let's verify the consistency of this structure across related files:
✅ Verification successful
The preRequisite structure is consistently implemented across configuration files
The verification confirms that the preRequisite structure is properly implemented in both c1-contents-config.ts and fyle-contents-config.ts with identical properties and consistent values, maintaining uniformity across the codebase for the QuickBooks integration onboarding feature.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the preRequisite structure is consistently implemented in related files
# Search for preRequisite structure in configuration files
rg -U "preRequisite.*\{[\s\S]*?stepName.*\n.*configurationHeaderText.*\n.*configurationSubHeaderText" src/app/branding/
Length of output: 933
src/app/branding/c1-contents-config.ts (2)
13-17: LGTM! The preRequisite section is well-structured.
The new section follows a consistent naming pattern and provides clear, informative text for guiding users through the QuickBooks integration setup.
13-17: Verify the text content for accessibility.
The configurationSubHeaderText should be verified to ensure it meets accessibility guidelines for readability and clarity.
✅ Verification successful
Let me gather more context about the text patterns used in configuration headers to ensure consistency and accessibility.
The text content follows accessibility best practices and maintains consistency
The text "Begin your QuickBooks integration by completing the following steps" is:
- Clear and direct in its purpose
- Uses active voice and simple language
- Consistent with other similar configuration texts in the codebase
- Follows a standard pattern used across different integration configurations
- Provides clear context and next steps to users
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if similar text patterns exist in other configuration sections
# and verify consistency in language and structure
rg -i "begin.*integration.*following.*steps"
Length of output: 356
Script:
#!/bin/bash
# Check all configurationHeaderText and configurationSubHeaderText patterns
# to verify consistency in language structure across different configurations
rg -B1 -A1 "configurationHeaderText|configurationSubHeaderText"
Length of output: 6261
src/app/branding/fyle-contents-config.ts (1)
13-17: LGTM! New preRequisite section is well-structured.
The new preRequisite configuration object is properly structured and contains clear, descriptive text for the QuickBooks integration setup.
| username: string, | ||
| password: string, |
There was a problem hiding this comment.
Consider implementing secure password handling
The model exposes sensitive credentials as plain strings. Consider implementing proper encryption or secure handling mechanisms for the password field.
Recommendations:
- Never expose plain passwords in API responses
- Consider implementing a more secure authentication mechanism
- If password must be stored, ensure it's properly hashed/encrypted at the data layer
| completed: false, | ||
| step: this.brandingContent.preRequisite.stepName, | ||
| icon: 'link-vertical-medium', | ||
| route: '/integrations/qbo/onboarding/pre_requisite', |
There was a problem hiding this comment.
Fix incorrect integration routes
The routes contain 'qbo' (QuickBooks Online) but this is a QBD (QuickBooks Desktop) integration model. This mismatch could cause routing issues.
Apply this pattern to fix all routes:
- route: '/integrations/qbo/onboarding/pre_requisite',
+ route: '/integrations/qbd/onboarding/pre_requisite',Also applies to: 37-37, 45-45, 53-53, 61-61
| private onboardingSteps: OnboardingStepper[] = [ | ||
| { | ||
| active: false, | ||
| completed: false, | ||
| step: this.brandingContent.preRequisite.stepName, | ||
| icon: 'link-vertical-medium', | ||
| route: '/integrations/qbo/onboarding/pre_requisite', | ||
| styleClasses: ['step-name-connector--text'] | ||
| }, | ||
| { | ||
| active: false, | ||
| completed: false, | ||
| step: this.brandingContent.connector.stepName, | ||
| icon: 'link-vertical-medium', | ||
| route: '/integrations/qbo/onboarding/connector', | ||
| styleClasses: ['step-name-connector--text'] | ||
| }, | ||
| { | ||
| active: false, | ||
| completed: false, | ||
| step: this.brandingContent.exportSetting.stepName, | ||
| icon: 'arrow-tail-up-medium', | ||
| route: '/integrations/qbo/onboarding/export_settings', | ||
| styleClasses: ['step-name-export--text'] | ||
| }, | ||
| { | ||
| active: false, | ||
| completed: false, | ||
| step: this.brandingContent.importSetting.stepName, | ||
| icon: 'arrow-tail-down-medium', | ||
| route: '/integrations/qbo/onboarding/import_settings', | ||
| styleClasses: ['step-name-export--text'] | ||
| }, | ||
| { | ||
| active: false, | ||
| completed: false, | ||
| step: this.brandingContent.advancedSettings.stepName, | ||
| icon: 'gear-medium', | ||
| route: '/integrations/qbo/onboarding/advanced_settings', | ||
| styleClasses: ['step-name-advanced--text'] | ||
| } | ||
| ]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance type safety and consistency
A few suggestions to improve the code:
- The onboardingSteps array should be readonly to prevent accidental modifications
- Style classes are inconsistent between steps (connector vs export)
- Icon names could be typed for better maintainability
Consider these improvements:
- private onboardingSteps: OnboardingStepper[] = [
+ private readonly onboardingSteps: ReadonlyArray<OnboardingStepper> = [// Add type for icon names
+type IconName = 'link-vertical-medium' | 'arrow-tail-up-medium' | 'arrow-tail-down-medium' | 'gear-medium';Also, consider unifying the style classes naming convention:
- styleClasses: ['step-name-connector--text']
+ styleClasses: ['step-name--text']Committable suggestion skipped: line range outside the PR's diff.
| getOnboardingSteps(currentStep: string, onboardingState: QbdDirectOnboardingState): OnboardingStepper[] { | ||
| this.onboardingSteps.forEach(step => { | ||
| if (step.step.toLowerCase() === currentStep.toLowerCase()) { | ||
| step.active = true; | ||
| } else { | ||
| step.active = false; | ||
| } | ||
| }); | ||
|
|
||
| for (let index = this.onboardingStateStepMap[onboardingState] - 1; index > 0; index--) { | ||
| this.onboardingSteps[index - 1].completed = true; | ||
| } | ||
|
|
||
| return this.onboardingSteps; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve method robustness and maintainability
The method has several areas for improvement:
- It mutates the private onboardingSteps array
- No validation for currentStep matching a valid step
- Case-insensitive comparison could be extracted
Consider this improved implementation:
+ private isStepMatch(step: string, currentStep: string): boolean {
+ return step.toLowerCase() === currentStep.toLowerCase();
+ }
+
getOnboardingSteps(currentStep: string, onboardingState: QbdDirectOnboardingState): OnboardingStepper[] {
+ // Validate currentStep
+ if (!this.onboardingSteps.some(step => this.isStepMatch(step.step, currentStep))) {
+ throw new Error(`Invalid step: ${currentStep}`);
+ }
+
+ // Create a copy to avoid mutations
+ const steps = [...this.onboardingSteps];
+
- this.onboardingSteps.forEach(step => {
+ steps.forEach(step => {
- if (step.step.toLowerCase() === currentStep.toLowerCase()) {
+ if (this.isStepMatch(step.step, currentStep)) {
step.active = true;
} else {
step.active = false;
}
});
for (let index = this.onboardingStateStepMap[onboardingState] - 1; index > 0; index--) {
- this.onboardingSteps[index - 1].completed = true;
+ steps[index - 1].completed = true;
}
- return this.onboardingSteps;
+ return steps;
}📝 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.
| getOnboardingSteps(currentStep: string, onboardingState: QbdDirectOnboardingState): OnboardingStepper[] { | |
| this.onboardingSteps.forEach(step => { | |
| if (step.step.toLowerCase() === currentStep.toLowerCase()) { | |
| step.active = true; | |
| } else { | |
| step.active = false; | |
| } | |
| }); | |
| for (let index = this.onboardingStateStepMap[onboardingState] - 1; index > 0; index--) { | |
| this.onboardingSteps[index - 1].completed = true; | |
| } | |
| return this.onboardingSteps; | |
| } | |
| private isStepMatch(step: string, currentStep: string): boolean { | |
| return step.toLowerCase() === currentStep.toLowerCase(); | |
| } | |
| getOnboardingSteps(currentStep: string, onboardingState: QbdDirectOnboardingState): OnboardingStepper[] { | |
| // Validate currentStep | |
| if (!this.onboardingSteps.some(step => this.isStepMatch(step.step, currentStep))) { | |
| throw new Error(`Invalid step: ${currentStep}`); | |
| } | |
| // Create a copy to avoid mutations | |
| const steps = [...this.onboardingSteps]; | |
| steps.forEach(step => { | |
| if (this.isStepMatch(step.step, currentStep)) { | |
| step.active = true; | |
| } else { | |
| step.active = false; | |
| } | |
| }); | |
| for (let index = this.onboardingStateStepMap[onboardingState] - 1; index > 0; index--) { | |
| steps[index - 1].completed = true; | |
| } | |
| return steps; | |
| } |
* 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]>
* 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]>
Description
feat: onboarding basic setup
Clickup
https://app.clickup.com/t/86cwzcd5g
Summary by CodeRabbit
Release Notes
preRequisitesection in the QuickBooks and Fyle configuration settings to guide users through initial setup steps.QbdDirectOnboardingModelclass to manage onboarding steps and states more effectively.These updates aim to improve user experience and clarity during the integration setup.