-
Notifications
You must be signed in to change notification settings - Fork 0
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: Prod fixes of QBD direct #1118
Conversation
WalkthroughThe changes in this pull request involve modifications to various branding configuration files and components related to the QBD (QuickBooks Desktop) integration. Key alterations include restructuring the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
There was a problem hiding this 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 and nitpick comments (2)
src/app/shared/components/configuration/configuration-select-field/configuration-select-field.component.html (1)
100-101
: Improve template readability and maintainabilityWhile the logic is correct, the template conditions are complex and could be simplified for better maintainability.
Consider these improvements:
- Move the complex conditions to the component class:
- <span *ngIf="(appName === AppName.QBD_DIRECT && form.controls.creditCardExportType.value === 'CREDIT_CARD_PURCHASE' && formControllerName === 'creditCardExportGroup') || (appName !== AppName.QBD_DIRECT)">Auto-selected based on your export module</span> - <span *ngIf="(formControllerName === 'reimbursableExportGroup' && form.controls.reimbursableExportType.value !== 'BILL' && appName === AppName.QBD_DIRECT) || (appName === AppName.QBD_DIRECT && form.controls.creditCardExportType.value !== 'CREDIT_CARD_PURCHASE' && formControllerName === 'creditCardExportGroup')"> Auto-selected when your default credit account is set to an Accounts Payable account</span> + <span *ngIf="shouldShowExportModuleMessage()">Auto-selected based on your export module</span> + <span *ngIf="shouldShowApAccountMessage()">{{getApAccountMessage()}}</span>
- Add these methods to the component class:
private readonly CREDIT_CARD_PURCHASE = 'CREDIT_CARD_PURCHASE'; private readonly BILL = 'BILL'; shouldShowExportModuleMessage(): boolean { return ( (this.appName === AppName.QBD_DIRECT && this.form.controls.creditCardExportType.value === this.CREDIT_CARD_PURCHASE && this.formControllerName === 'creditCardExportGroup') || this.appName !== AppName.QBD_DIRECT ); } shouldShowApAccountMessage(): boolean { return ( (this.formControllerName === 'reimbursableExportGroup' && this.form.controls.reimbursableExportType.value !== this.BILL && this.appName === AppName.QBD_DIRECT) || (this.appName === AppName.QBD_DIRECT && this.form.controls.creditCardExportType.value !== this.CREDIT_CARD_PURCHASE && this.formControllerName === 'creditCardExportGroup') ); } getApAccountMessage(): string { return 'Auto-selected when your default credit account is set to an Accounts Payable account'; }
- Alternatively, consider using ng-template for better organization:
<ng-container *ngTemplateOutlet="autoSelectionMessage"></ng-container> <ng-template #autoSelectionMessage> <ng-container [ngSwitch]="true"> <span *ngSwitchCase="shouldShowExportModuleMessage()"> Auto-selected based on your export module </span> <span *ngSwitchCase="shouldShowApAccountMessage()"> {{getApAccountMessage()}} </span> </ng-container> </ng-template>src/app/integrations/qbd-direct/qbd-direct-onboarding/qbd-direct-onboarding-connector/qbd-direct-onboarding-connector.component.ts (1)
Line range hint
90-96
: Fix loading state management and add error handling.The code has several issues:
- Loading state is incorrectly set to false before the HTTP request completes
- Missing error handling for the HTTP request
-this.qbdDirectConnectorService.postQbdDirectConntion({file_location: filePath}).subscribe((connectionResponse: QbdConnectorGet) => { +this.qbdDirectConnectorService.postQbdDirectConntion({file_location: filePath}).subscribe({ + next: (connectionResponse: QbdConnectorGet) => { this.password = connectionResponse.password; this.xmlFileContent = connectionResponse.qwc; this.triggerManualDownload(); this.showDownloadLink = true; + this.isDownloadfileLoading = false; + }, + error: (error) => { + this.toastService.displayToastMessage(ToastSeverity.ERROR, 'Failed to process file path. Please try again.'); + this.isDownloadfileLoading = false; + } }); -this.isDownloadfileLoading = false;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
commits.tsv
is excluded by!**/*.tsv
📒 Files selected for processing (9)
src/app/branding/c1-branding-config.ts
(0 hunks)src/app/branding/fyle-branding-config.ts
(2 hunks)src/app/core/models/branding/kb-article.model.ts
(0 hunks)src/app/integrations/qbd-direct/qbd-direct-main/qbd-direct-dashboard/qbd-direct-dashboard.component.ts
(2 hunks)src/app/integrations/qbd-direct/qbd-direct-main/qbd-direct-mapping/qbd-direct-base-mapping/qbd-direct-base-mapping.component.ts
(2 hunks)src/app/integrations/qbd-direct/qbd-direct-onboarding/qbd-direct-onboarding-connector/qbd-direct-onboarding-connector.component.ts
(1 hunks)src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-advanced-settings/qbd-direct-advanced-settings.component.html
(3 hunks)src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-advanced-settings/qbd-direct-advanced-settings.component.ts
(4 hunks)src/app/shared/components/configuration/configuration-select-field/configuration-select-field.component.html
(1 hunks)
💤 Files with no reviewable changes (2)
- src/app/core/models/branding/kb-article.model.ts
- src/app/branding/c1-branding-config.ts
🔇 Additional comments (7)
src/app/integrations/qbd-direct/qbd-direct-main/qbd-direct-mapping/qbd-direct-base-mapping/qbd-direct-base-mapping.component.ts (1)
110-110
: Verify the impact of chart of accounts changes
The conditional logic for chartOfAccounts
assignment has been modified to use either the imported chart of accounts or a predefined list based on import_account_as_category
. This change could affect existing mappings.
✅ Verification successful
Let me gather more information about the impact by checking how the chart of accounts is used in mappings and any dependencies.
Based on the analysis of the shell script results, I can now provide a final response:
Chart of accounts change is safe and properly handled
The modification to use either imported chart of accounts or predefined list is safe because:
- The change is consistently implemented across both mapping and dashboard components
- The predefined list from
getChartOfAccountTypesList()
serves as a fallback whenimport_account_as_category
is false - Existing mappings will continue to work as the chart of accounts options are dynamically loaded during component initialization
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for existing mappings that might be affected by the chart of accounts changes
# Search for mapping references to chart_of_accounts
rg -A 5 "chart_of_accounts" --type ts
# Search for any existing mapping configurations
ast-grep --pattern 'chartOfAccounts = $_'
Length of output: 5403
Script:
#!/bin/bash
# Check for usage of chartOfAccounts in mapping logic and UI
rg -A 10 "chartOfAccounts" --type ts
# Check for any mapping-related components that might use these values
rg -A 5 "getChartOfAccountTypesList" --type ts
# Look for any mapping storage or persistence logic
ast-grep --pattern 'class $_ extends QbdDirectBaseMappingComponent'
Length of output: 30694
src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-advanced-settings/qbd-direct-advanced-settings.component.html (2)
92-97
: LGTM! Enhanced user experience with preview section
The addition of a preview section for the top-level memo structure improves user understanding by providing immediate visual feedback.
124-124
: Verify the removal of redirectLink
The removal of the redirectLink from the skip export toggle field might affect the help documentation accessibility.
src/app/integrations/qbd-direct/qbd-direct-main/qbd-direct-dashboard/qbd-direct-dashboard.component.ts (1)
155-155
: LGTM! Consistent implementation of chart of accounts logic
The implementation maintains consistency with the base mapping component, using the same conditional logic for chartOfAccounts
assignment based on import_account_as_category
.
src/app/branding/fyle-branding-config.ts (2)
70-70
: LGTM: URL update maintains consistency with other article links.
The URL structure follows the established pattern using brandingConfig.helpArticleDomain
.
129-132
: Verify beta documentation readiness.
The URLs point to a beta integration article. Ensure that the documentation is complete and accurate before releasing to production.
src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-advanced-settings/qbd-direct-advanced-settings.component.ts (1)
67-67
: LGTM: Consistent article reference.
The redirectLink
property correctly references the advanced settings article using the branding configuration.
// No empty input test, test for non whitespaces like spaces, tabs, etc. | ||
const filePathRegex = /^\s*\S.*$/; | ||
this.isCompanyPathInvalid = filePathRegex.test(filePath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider stricter file path validation.
The current regex only checks for non-empty input, which might be too permissive. Consider validating the file extension to ensure it's a valid QuickBooks file path.
-const filePathRegex = /^\s*\S.*$/;
+const filePathRegex = /^\s*\S.*\.qbw$/i;
📝 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.
// No empty input test, test for non whitespaces like spaces, tabs, etc. | |
const filePathRegex = /^\s*\S.*$/; | |
this.isCompanyPathInvalid = filePathRegex.test(filePath); | |
// No empty input test, test for non whitespaces like spaces, tabs, etc. | |
const filePathRegex = /^\s*\S.*\.qbw$/i; | |
this.isCompanyPathInvalid = filePathRegex.test(filePath); |
private createTopMemoStructureWatcher(): void { | ||
this.memoStructure = this.advancedSettingsForm.value.topMemoStructure; | ||
this.topMemoPreviewText = QbdDirectAdvancedSettingsModel.formatMemoPreview(this.memoStructure, this.defaultTopMemoOptions)[0]; | ||
this.advancedSettingsForm.controls.topMemoStructure.valueChanges.subscribe((memoChanges) => { | ||
this.memoStructure = memoChanges; | ||
this.topMemoPreviewText = QbdDirectAdvancedSettingsModel.formatMemoPreview(this.memoStructure, this.defaultTopMemoOptions)[0]; | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Prevent potential memory leak in subscription.
The subscription to topMemoStructure
valueChanges should be cleaned up when the component is destroyed.
+private topMemoSubscription: Subscription;
private createTopMemoStructureWatcher(): void {
this.memoStructure = this.advancedSettingsForm.value.topMemoStructure;
this.topMemoPreviewText = QbdDirectAdvancedSettingsModel.formatMemoPreview(this.memoStructure, this.defaultTopMemoOptions)[0];
- this.advancedSettingsForm.controls.topMemoStructure.valueChanges.subscribe((memoChanges) => {
+ this.topMemoSubscription = this.advancedSettingsForm.controls.topMemoStructure.valueChanges.subscribe((memoChanges) => {
this.memoStructure = memoChanges;
this.topMemoPreviewText = QbdDirectAdvancedSettingsModel.formatMemoPreview(this.memoStructure, this.defaultTopMemoOptions)[0];
});
}
+ngOnDestroy(): void {
+ if (this.topMemoSubscription) {
+ this.topMemoSubscription.unsubscribe();
+ }
+}
Committable suggestion skipped: line range outside the PR's diff.
const filePathRegex = /^\s*\S.*$/; | ||
this.isCompanyPathInvalid = filePathRegex.test(filePath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we even need regex for simple comparison, can just do if (filePath) {...}
There was a problem hiding this 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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/app/integrations/qbd-direct/qbd-direct-onboarding/qbd-direct-onboarding-connector/qbd-direct-onboarding-connector.component.ts
(1 hunks)
🧰 Additional context used
🪛 eslint
src/app/integrations/qbd-direct/qbd-direct-onboarding/qbd-direct-onboarding-connector/qbd-direct-onboarding-connector.component.ts
[error] 86-86: Expected space(s) after "if".
(keyword-spacing)
this.isDownloadfileLoading = true; | ||
this.isCompanyPathInvalid = false; | ||
this.qbdDirectConnectorService.postQbdDirectConntion({file_location: filePath}).subscribe((connectionResponse: QbdConnectorGet) => { | ||
this.password = connectionResponse.password; | ||
this.xmlFileContent = connectionResponse.qwc; | ||
this.triggerManualDownload(); | ||
this.showDownloadLink = true; | ||
}); | ||
this.isDownloadfileLoading = false; | ||
}); | ||
this.isDownloadfileLoading = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix loading state management
The loading state is incorrectly managed. Setting isDownloadfileLoading = false
outside the subscription will execute before the API call completes, leading to incorrect UI state.
if(filePath) {
this.isDownloadfileLoading = true;
this.isCompanyPathInvalid = false;
this.qbdDirectConnectorService.postQbdDirectConntion({file_location: filePath}).subscribe((connectionResponse: QbdConnectorGet) => {
this.password = connectionResponse.password;
this.xmlFileContent = connectionResponse.qwc;
this.triggerManualDownload();
this.showDownloadLink = true;
+ this.isDownloadfileLoading = false;
});
- this.isDownloadfileLoading = false;
}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
this.isDownloadfileLoading = true; | |
this.isCompanyPathInvalid = false; | |
this.qbdDirectConnectorService.postQbdDirectConntion({file_location: filePath}).subscribe((connectionResponse: QbdConnectorGet) => { | |
this.password = connectionResponse.password; | |
this.xmlFileContent = connectionResponse.qwc; | |
this.triggerManualDownload(); | |
this.showDownloadLink = true; | |
}); | |
this.isDownloadfileLoading = false; | |
}); | |
this.isDownloadfileLoading = false; | |
this.isDownloadfileLoading = true; | |
this.isCompanyPathInvalid = false; | |
this.qbdDirectConnectorService.postQbdDirectConntion({file_location: filePath}).subscribe((connectionResponse: QbdConnectorGet) => { | |
this.password = connectionResponse.password; | |
this.xmlFileContent = connectionResponse.qwc; | |
this.triggerManualDownload(); | |
this.showDownloadLink = true; | |
this.isDownloadfileLoading = false; | |
}); |
if(filePath) { | ||
this.isDownloadfileLoading = true; | ||
this.isCompanyPathInvalid = false; | ||
this.qbdDirectConnectorService.postQbdDirectConntion({file_location: filePath}).subscribe((connectionResponse: QbdConnectorGet) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider basic path validation while maintaining simplicity
While simplifying the validation aligns with the requirements, consider adding basic sanity checks to prevent obvious issues:
-if(filePath) {
+if (filePath?.trim()) {
this.isDownloadfileLoading = true;
this.isCompanyPathInvalid = false;
- this.qbdDirectConnectorService.postQbdDirectConntion({file_location: filePath}).subscribe((connectionResponse: QbdConnectorGet) => {
+ this.qbdDirectConnectorService.postQbdDirectConntion({file_location: filePath.trim()}).subscribe((connectionResponse: QbdConnectorGet) => {
Also, fix the spacing after if
to maintain consistent code style.
📝 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.
if(filePath) { | |
this.isDownloadfileLoading = true; | |
this.isCompanyPathInvalid = false; | |
this.qbdDirectConnectorService.postQbdDirectConntion({file_location: filePath}).subscribe((connectionResponse: QbdConnectorGet) => { | |
if (filePath?.trim()) { | |
this.isDownloadfileLoading = true; | |
this.isCompanyPathInvalid = false; | |
this.qbdDirectConnectorService.postQbdDirectConntion({file_location: filePath.trim()}).subscribe((connectionResponse: QbdConnectorGet) => { |
🧰 Tools
🪛 eslint
[error] 86-86: Expected space(s) after "if".
(keyword-spacing)
this.qbdDirectConnectorService.postQbdDirectConntion({file_location: filePath}).subscribe((connectionResponse: QbdConnectorGet) => { | ||
this.password = connectionResponse.password; | ||
this.xmlFileContent = connectionResponse.qwc; | ||
this.triggerManualDownload(); | ||
this.showDownloadLink = true; | ||
}); | ||
this.isDownloadfileLoading = false; | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add error handling to API call
The API subscription lacks error handling, which could leave the UI in a loading state if an error occurs.
this.qbdDirectConnectorService.postQbdDirectConntion({file_location: filePath}).subscribe(
- (connectionResponse: QbdConnectorGet) => {
+ {
+ next: (connectionResponse: QbdConnectorGet) => {
this.password = connectionResponse.password;
this.xmlFileContent = connectionResponse.qwc;
this.triggerManualDownload();
this.showDownloadLink = true;
this.isDownloadfileLoading = false;
+ },
+ error: (error) => {
+ this.isDownloadfileLoading = false;
+ this.toastService.displayToastMessage(ToastSeverity.ERROR, 'Failed to establish QuickBooks Desktop connection');
+ }
+ }
);
Committable suggestion skipped: line range outside the PR's diff.
|
* fix: remove validation temporarily (#1111) * fix: add a default bank account field for CCC expenses * fix: add missing options to bank accounts on page init * fix: dynamic content for xero customize settings (#1112) * fix: update sublabel key to avoid build fail (#1116) * fix: Prod fixes of QBD direct (#1118) * fix bugs (#1119) * refactor: capitalization * fix: only ccc exports not being saved (#1121) --------- Co-authored-by: Ashwin Thanaraj <[email protected]> Co-authored-by: Nilesh Pant <[email protected]> Co-authored-by: Dhaarani <[email protected]> Co-authored-by: Anish Kr Singh <[email protected]>
* fix: restrict JE modules to group by expense only * fix: add a default bank account field for CCC expenses (#1114) * fix: remove validation temporarily (#1111) * fix: add a default bank account field for CCC expenses * fix: add missing options to bank accounts on page init * fix: dynamic content for xero customize settings (#1112) * fix: update sublabel key to avoid build fail (#1116) * fix: Prod fixes of QBD direct (#1118) * fix bugs (#1119) * refactor: capitalization * fix: only ccc exports not being saved (#1121) --------- Co-authored-by: Ashwin Thanaraj <[email protected]> Co-authored-by: Nilesh Pant <[email protected]> Co-authored-by: Dhaarani <[email protected]> Co-authored-by: Anish Kr Singh <[email protected]> --------- Co-authored-by: Ashwin Thanaraj <[email protected]> Co-authored-by: Nilesh Pant <[email protected]> Co-authored-by: Dhaarani <[email protected]> Co-authored-by: Anish Kr Singh <[email protected]>
* fix: initialize chart of accounts multiselect when there is no api response (#1110) * fix: remove the posted at date option for ccc expenses grouped by report (#1105) * fix: update login error flow and fix redirect url (#1117) * fix: restrict JE modules to group by expense only (#1113) * fix: restrict JE modules to group by expense only * fix: add a default bank account field for CCC expenses (#1114) * fix: remove validation temporarily (#1111) * fix: add a default bank account field for CCC expenses * fix: add missing options to bank accounts on page init * fix: dynamic content for xero customize settings (#1112) * fix: update sublabel key to avoid build fail (#1116) * fix: Prod fixes of QBD direct (#1118) * fix bugs (#1119) * refactor: capitalization * fix: only ccc exports not being saved (#1121) --------- Co-authored-by: Ashwin Thanaraj <[email protected]> Co-authored-by: Nilesh Pant <[email protected]> Co-authored-by: Dhaarani <[email protected]> Co-authored-by: Anish Kr Singh <[email protected]> --------- Co-authored-by: Ashwin Thanaraj <[email protected]> Co-authored-by: Nilesh Pant <[email protected]> Co-authored-by: Dhaarani <[email protected]> Co-authored-by: Anish Kr Singh <[email protected]> --------- Co-authored-by: Ashwin Thanaraj <[email protected]> Co-authored-by: Nilesh Pant <[email protected]> Co-authored-by: Dhaarani <[email protected]> Co-authored-by: Anish Kr Singh <[email protected]>
Description
fix: Prod fixes of QBD direct
app changes
remove error message shown in company path, check only for not empty string - p0
commits.tsv - p0
category mappings - p0
if category import is on -> today's behavior is right
if category import is off -> send all account types in detail query param
export settings -> reimbursable/ccc grouping help text fix - p1
advanced settings top level preview - p2
updated the KB article as well
Clickup
https://app.clickup.com
Export settings
Advanced settings
Mapping
when import category is off
when import category is on
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor
Style