feat: Qbd direct je changes #1091
Conversation
* 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]>
#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)
WalkthroughThe pull request introduces modifications to several configuration and model files within the application. Key updates include changes to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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: 9
🧹 Outside diff range and nitpick comments (8)
src/app/integrations/qbd-direct/qbd-direct-main/qbd-direct-mapping/qbd-direct-base-mapping/qbd-direct-base-mapping.component.ts (3)
51-54: Consider adding access modifiers to class propertiesFor better encapsulation, consider adding appropriate access modifiers (public/private) to the new properties based on their usage in the template.
- nameInJE: NameInJournalEntry; + private nameInJE: NameInJournalEntry; - chartOfAccounts: string[]; + private chartOfAccounts: string[];
74-93: Add JSDoc documentation to new methodsConsider adding JSDoc documentation to improve code maintainability and developer experience.
/** * Updates destination options based on the provided account types * @param detailAccountType - Optional array of account types to filter by */ destinationOptionsWatcher(detailAccountType?: string[]): void /** * Handles destination options for corporate credit card accounts * based on expense object type and employee field mapping */ cccAccountOptions(): void /** * Updates destination options based on chart of accounts */ categoryOptions(): void
Line range hint
1-134: Consider implementing a state management solutionGiven the complexity of the mapping logic and the number of interdependent properties, consider implementing a state management solution (e.g., NgRx, RxJS BehaviorSubjects) to:
- Better manage the component's state
- Improve testability
- Make the data flow more predictable
- Separate data management from presentation logic
This would help reduce the component's complexity and make it more maintainable in the long run.
src/app/core/services/common/mapping.service.ts (1)
175-175: Consider extracting the params interfaceThe params interface is quite complex and defined inline. Consider extracting it to a separate interface definition for better maintainability and reusability.
interface PaginatedDestinationAttributesParams { limit: number; offset: number; attribute_type: string; active?: boolean; value__icontains?: string; value?: string; display_name__in?: string; detail__account_type__in?: string[]; }src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-export-settings/qbd-direct-export-settings.component.ts (1)
Line range hint
1-300: Consider architectural improvements for better maintainabilityThe component has grown complex with multiple responsibilities. Consider these architectural improvements:
- Extract form handling logic into a separate service
- Create an enum or constant file for account types
- Consider splitting into smaller components (e.g., separate components for reimbursable and credit card settings)
- Add unit tests to cover the complex conditional logic
This will improve maintainability, testability, and make the code more modular.
src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-export-settings/qbd-direct-export-settings.component.html (2)
159-172: Add aria-label for accessibilityThe new journal entry name selection field is well-implemented with proper feature flag and export type conditions. Consider adding an aria-label for better accessibility.
<app-configuration-select-field [form]="exportSettingsForm" [isFieldMandatory]="true" [showClearIcon]="true" [mandatoryErrorListName]="'Name in which Journal Entry should export'" [label]="brandingContent.corporateCard.creditCardExpenseNameinJELabel" [subLabel]="brandingContent.corporateCard.creditCardExpenseNameinJESubLabel" [options]="nameInJEOptions" [iconPath]="'list'" [placeholder]="'Select a name'" + [attr.aria-label]="brandingContent.corporateCard.creditCardExpenseNameinJELabel" [formControllerName]="'nameInJE'"> </app-configuration-select-field>
242-242: Simplify the disable conditionThe current disable condition uses an unnecessary ternary operator. It can be simplified since the condition already returns a boolean.
-[isDisabled]="exportSettingsForm.get('creditCardExportType')?.value === QBDCorporateCreditCardExpensesObject.CREDIT_CARD_PURCHASE || exportSettingsForm.get('creditCardExportType')?.value === QBDCorporateCreditCardExpensesObject.JOURNAL_ENTRY ? true : false" +[isDisabled]="exportSettingsForm.get('creditCardExportType')?.value === QBDCorporateCreditCardExpensesObject.CREDIT_CARD_PURCHASE || exportSettingsForm.get('creditCardExportType')?.value === QBDCorporateCreditCardExpensesObject.JOURNAL_ENTRY"src/app/branding/fyle-contents-config.ts (1)
Line range hint
1-500: Consider splitting this configuration file into smaller modules.The file contains a large configuration object with nested settings for multiple platforms. This could lead to maintenance challenges as more platforms or configurations are added.
Consider splitting the configuration into separate files per platform:
- export const fyleContents = { ... } + // src/app/branding/platforms/qbd-contents.config.ts + export const qbdContents = { ... } + + // src/app/branding/platforms/netsuite-contents.config.ts + export const netsuiteContents = { ... } + + // src/app/branding/fyle-contents-config.ts + import { qbdContents } from './platforms/qbd-contents.config'; + import { netsuiteContents } from './platforms/netsuite-contents.config'; + + export const fyleContents = { + qbd_direct: qbdContents, + netsuite: netsuiteContents, + // ... import and include other platforms + };This would:
- Improve maintainability by reducing file size
- Make it easier to manage platform-specific changes
- Enable better code organization and separation of concerns
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
src/app/branding/c1-contents-config.ts(1 hunks)src/app/branding/fyle-contents-config.ts(1 hunks)src/app/core/models/qbd-direct/qbd-direct-configuration/qbd-direct-export-settings.model.ts(3 hunks)src/app/core/services/common/mapping.service.ts(2 hunks)src/app/integrations/qbd-direct/qbd-direct-main/qbd-direct-mapping/qbd-direct-base-mapping/qbd-direct-base-mapping.component.ts(3 hunks)src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-export-settings/qbd-direct-export-settings.component.html(3 hunks)src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-export-settings/qbd-direct-export-settings.component.ts(1 hunks)
🔇 Additional comments (6)
src/app/integrations/qbd-direct/qbd-direct-main/qbd-direct-mapping/qbd-direct-base-mapping/qbd-direct-base-mapping.component.ts (2)
8-8: LGTM: Import statements are properly organized
The new imports for NameInJournalEntry and QbdDirectImportSettingsService are correctly added and align with their usage in the component.
Also applies to: 15-15
60-61: LGTM: Service injection follows Angular conventions
The QbdDirectImportSettingsService is properly injected following Angular's dependency injection patterns.
src/app/core/services/common/mapping.service.ts (1)
Line range hint 173-197: Verify API endpoint compatibility and method usage
Please ensure:
- The API endpoint
/workspaces/{id}/mappings/paginated_destination_attributes/supports the newdetail__account_type__infilter parameter - All callers of this method have been updated to handle the new optional parameter
src/app/core/models/qbd-direct/qbd-direct-configuration/qbd-direct-export-settings.model.ts (2)
175-176: LGTM: Logical addition of mandatory field check
The new case correctly enforces the requirement of having a default credit card account name when the export type is set to credit card purchase.
186-186: Verify UI handling of conditional validation
The validation logic changes look good - moving defaultCreditCardAccountName from general credit card expense validation to being specifically required for CREDIT_CARD_PURCHASE type makes sense. However, we should verify that all UI components correctly handle this validation change.
Let's verify the UI components that might be affected:
Also applies to: 200-200
✅ Verification successful
Based on the search results, I can now generate the final response:
UI components correctly handle the conditional validation
The UI components properly handle the conditional validation for defaultCreditCardAccountName:
- In
qbd-direct-export-settings.component.html, the field is conditionally rendered using*ngIf="exportSettingsForm.get('creditCardExportType')?.value && helperService.isFieldRequired(exportSettingsForm, 'defaultCreditCardAccountName')", which ensures it's only shown when required - The form control is correctly bound with
[formControllerName]="'defaultCreditCardAccountName'"and includes proper labeling based on the export type - The validation logic in the model aligns with the UI implementation, making
defaultCreditCardAccountNamemandatory only forCREDIT_CARD_PURCHASEtype
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find UI components that reference defaultCreditCardAccountName
# Search for components using defaultCreditCardAccountName
echo "=== Components referencing defaultCreditCardAccountName ==="
rg -t typescript -t html "defaultCreditCardAccountName"
# Search for form validation related to credit card exports
echo "=== Form validation logic for credit card exports ==="
rg -t typescript "creditCardExportType.*validation|validate.*creditCard"
Length of output: 446
Script:
#!/bin/bash
# Let's try with ripgrep without file type filters
echo "=== Components referencing defaultCreditCardAccountName ==="
rg "defaultCreditCardAccountName"
echo -e "\n=== Form validation and credit card export logic ==="
rg "creditCardExportType|creditCard.*validation"
Length of output: 46031
src/app/branding/fyle-contents-config.ts (1)
61-61: LGTM! Text change improves clarity.
The label text has been simplified from "for adding Journal Entry" to "Select the Default Credit Account for adding", which maintains consistency with similar labels throughout the configuration.
| destinationOptionsWatcher(detailAccountType?: string[]): void { | ||
| this.mappingService.getPaginatedDestinationAttributes(this.destinationField, undefined, this.displayName, '', detailAccountType).subscribe((responses) => { | ||
| this.destinationOptions = responses.results as QbdDirectDestinationAttribute[]; | ||
| this.isLoading = false; | ||
| }); | ||
| } |
There was a problem hiding this comment.
Add error handling to the destination options API call
The subscription to getPaginatedDestinationAttributes should include error handling to manage API failures gracefully.
destinationOptionsWatcher(detailAccountType?: string[]): void {
this.mappingService.getPaginatedDestinationAttributes(this.destinationField, undefined, this.displayName, '', detailAccountType).subscribe((responses) => {
this.destinationOptions = responses.results as QbdDirectDestinationAttribute[];
this.isLoading = false;
- });
+ }, (error) => {
+ this.isLoading = false;
+ this.toastService.show('Error fetching destination options', 'danger');
+ console.error('Error fetching destination options:', error);
+ });
}Committable suggestion skipped: line range outside the PR's diff.
| cccAccpuntOptions(): void { | ||
| if (this.cccExpenseObject === QBDCorporateCreditCardExpensesObject.CREDIT_CARD_PURCHASE) { | ||
| this.destinationOptionsWatcher(['CreditCard']); | ||
| } else if (this.cccExpenseObject === QBDCorporateCreditCardExpensesObject.JOURNAL_ENTRY && this.employeeFieldMapping === FyleField.EMPLOYEE && this.nameInJE === NameInJournalEntry.EMPLOYEE) { | ||
| this.destinationOptionsWatcher(['Bank', 'CreditCard', 'OtherCurrentLiability', 'LongTermLiability']); | ||
| } else { | ||
| this.destinationOptionsWatcher(['Bank', 'AccountsPayable', 'CreditCard', 'OtherCurrentLiability', 'LongTermLiability']); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix typo in method name and simplify conditional logic
The method name has a typo, and the conditional logic could be simplified for better maintainability.
- cccAccpuntOptions(): void {
+ cccAccountOptions(): void {
+ const accountTypes = ['CreditCard'];
if (this.cccExpenseObject === QBDCorporateCreditCardExpensesObject.CREDIT_CARD_PURCHASE) {
- this.destinationOptionsWatcher(['CreditCard']);
+ this.destinationOptionsWatcher(accountTypes);
} else if (this.cccExpenseObject === QBDCorporateCreditCardExpensesObject.JOURNAL_ENTRY &&
this.employeeFieldMapping === FyleField.EMPLOYEE &&
this.nameInJE === NameInJournalEntry.EMPLOYEE) {
- this.destinationOptionsWatcher(['Bank', 'CreditCard', 'OtherCurrentLiability', 'LongTermLiability']);
+ accountTypes.push('Bank', 'OtherCurrentLiability', 'LongTermLiability');
+ this.destinationOptionsWatcher(accountTypes);
} else {
- this.destinationOptionsWatcher(['Bank', 'AccountsPayable', 'CreditCard', 'OtherCurrentLiability', 'LongTermLiability']);
+ accountTypes.push('Bank', 'AccountsPayable', 'OtherCurrentLiability', 'LongTermLiability');
+ this.destinationOptionsWatcher(accountTypes);
}
}📝 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.
| cccAccpuntOptions(): void { | |
| if (this.cccExpenseObject === QBDCorporateCreditCardExpensesObject.CREDIT_CARD_PURCHASE) { | |
| this.destinationOptionsWatcher(['CreditCard']); | |
| } else if (this.cccExpenseObject === QBDCorporateCreditCardExpensesObject.JOURNAL_ENTRY && this.employeeFieldMapping === FyleField.EMPLOYEE && this.nameInJE === NameInJournalEntry.EMPLOYEE) { | |
| this.destinationOptionsWatcher(['Bank', 'CreditCard', 'OtherCurrentLiability', 'LongTermLiability']); | |
| } else { | |
| this.destinationOptionsWatcher(['Bank', 'AccountsPayable', 'CreditCard', 'OtherCurrentLiability', 'LongTermLiability']); | |
| } | |
| } | |
| cccAccountOptions(): void { | |
| const accountTypes = ['CreditCard']; | |
| if (this.cccExpenseObject === QBDCorporateCreditCardExpensesObject.CREDIT_CARD_PURCHASE) { | |
| this.destinationOptionsWatcher(accountTypes); | |
| } else if (this.cccExpenseObject === QBDCorporateCreditCardExpensesObject.JOURNAL_ENTRY && this.employeeFieldMapping === FyleField.EMPLOYEE && this.nameInJE === NameInJournalEntry.EMPLOYEE) { | |
| accountTypes.push('Bank', 'OtherCurrentLiability', 'LongTermLiability'); | |
| this.destinationOptionsWatcher(accountTypes); | |
| } else { | |
| accountTypes.push('Bank', 'AccountsPayable', 'OtherCurrentLiability', 'LongTermLiability'); | |
| this.destinationOptionsWatcher(accountTypes); | |
| } | |
| } |
| forkJoin([ | ||
| this.expoerSettingService.getQbdExportSettings(), | ||
| this.exportSettingService.getQbdExportSettings(), | ||
| this.importSettingService.getImportSettings(), | ||
| this.mappingService.getMappingSettings() | ||
| ]).subscribe((responses) => { | ||
| this.reimbursableExpenseObject = responses[0].reimbursable_expense_export_type; | ||
| this.cccExpenseObject = responses[0].credit_card_expense_export_type; | ||
| this.employeeFieldMapping = (responses[0].employee_field_mapping as unknown as FyleField); | ||
|
|
||
| this.destinationField = this.getDestinationField(responses[0], responses[1].results); | ||
|
|
||
|
|
||
| this.mappingService.getPaginatedDestinationAttributes(this.destinationField, undefined, this.displayName).subscribe((responses) => { | ||
| this.destinationOptions = responses.results; | ||
| this.isLoading = false; | ||
| }); | ||
| this.nameInJE = responses[0].name_in_journal_entry; | ||
| this.chartOfAccounts = responses[1].import_settings.chart_of_accounts; | ||
|
|
||
| this.destinationField = this.getDestinationField(responses[0], responses[2].results); | ||
|
|
||
| if (this.sourceField === 'CORPORATE_CARD') { | ||
| this.cccAccpuntOptions(); | ||
| } else if (this.sourceField === 'CATEGORY') { | ||
| this.categoryOptions(); | ||
| } else { | ||
| this.destinationOptionsWatcher(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and method organization in setupPage
The setupPage method could benefit from better error handling and separation of concerns.
private setupPage(): void {
this.sourceField = decodeURIComponent(this.route.snapshot.params.source_field.toUpperCase());
forkJoin([
this.exportSettingService.getQbdExportSettings(),
this.importSettingService.getImportSettings(),
this.mappingService.getMappingSettings()
- ]).subscribe((responses) => {
+ ]).subscribe({
+ next: (responses) => {
this.reimbursableExpenseObject = responses[0].reimbursable_expense_export_type;
this.cccExpenseObject = responses[0].credit_card_expense_export_type;
this.employeeFieldMapping = (responses[0].employee_field_mapping as unknown as FyleField);
this.nameInJE = responses[0].name_in_journal_entry;
this.chartOfAccounts = responses[1].import_settings.chart_of_accounts;
this.destinationField = this.getDestinationField(responses[0], responses[2].results);
+ this.updateDestinationOptions();
+ },
+ error: (error) => {
+ this.isLoading = false;
+ this.toastService.show('Error loading mapping settings', 'danger');
+ console.error('Error in setupPage:', error);
+ }
});
}
+ private updateDestinationOptions(): void {
if (this.sourceField === 'CORPORATE_CARD') {
- this.cccAccpuntOptions();
+ this.cccAccountOptions();
} else if (this.sourceField === 'CATEGORY') {
this.categoryOptions();
} else {
this.destinationOptionsWatcher();
}
+ }Committable suggestion skipped: line range outside the PR's diff.
| @@ -170,9 +170,9 @@ export class MappingService { | |||
| return this.apiService.post(`/workspaces/${this.workspaceService.getWorkspaceId()}/mappings/`, mapping); | |||
| } | |||
|
|
|||
| getPaginatedDestinationAttributes(attributeType: string, value?: string, display_name?: string, appName?: string): Observable<PaginatedDestinationAttribute> { | |||
| getPaginatedDestinationAttributes(attributeType: string, value?: string, display_name?: string, appName?: string, detailed_accout_type?: string[]): Observable<PaginatedDestinationAttribute> { | |||
There was a problem hiding this comment.
Fix typo in parameter name
There's a typo in the parameter name: detailed_accout_type should be detailed_account_type.
- getPaginatedDestinationAttributes(attributeType: string, value?: string, display_name?: string, appName?: string, detailed_accout_type?: string[]): Observable<PaginatedDestinationAttribute> {
+ getPaginatedDestinationAttributes(attributeType: string, value?: string, display_name?: string, appName?: string, detailed_account_type?: string[]): Observable<PaginatedDestinationAttribute> {📝 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.
| getPaginatedDestinationAttributes(attributeType: string, value?: string, display_name?: string, appName?: string, detailed_accout_type?: string[]): Observable<PaginatedDestinationAttribute> { | |
| getPaginatedDestinationAttributes(attributeType: string, value?: string, display_name?: string, appName?: string, detailed_account_type?: string[]): Observable<PaginatedDestinationAttribute> { |
| if (detailed_accout_type) { | ||
| params.detail__account_type__in = detailed_accout_type; | ||
| } |
There was a problem hiding this comment.
Fix typo in condition block
The typo in the parameter name needs to be fixed in the condition block as well.
- if (detailed_accout_type) {
- params.detail__account_type__in = detailed_accout_type;
+ if (detailed_account_type) {
+ params.detail__account_type__in = detailed_account_type;📝 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 (detailed_accout_type) { | |
| params.detail__account_type__in = detailed_accout_type; | |
| } | |
| if (detailed_account_type) { | |
| params.detail__account_type__in = detailed_account_type; | |
| } |
| cccAccpuntOptions(cccExportType: string): DestinationAttribute[] { | ||
| if (cccExportType === QBDCorporateCreditCardExpensesObject.CREDIT_CARD_PURCHASE) { | ||
| return this.destinationOptionsWatcher(['CreditCard'], this.destinationAccounts); | ||
| } else if (cccExportType === QBDCorporateCreditCardExpensesObject.JOURNAL_ENTRY && this.exportSettingsForm.controls.employeeMapping.value === EmployeeFieldMapping.EMPLOYEE && this.exportSettingsForm.controls.nameInJE.value === EmployeeFieldMapping.EMPLOYEE) { | ||
| return this.destinationOptionsWatcher(['Bank', 'CreditCard', 'OtherCurrentLiability', 'LongTermLiability'], this.destinationAccounts); | ||
| } | ||
| return this.destinationOptionsWatcher(['Bank', 'AccountsPayable', 'CreditCard', 'OtherCurrentLiability', 'LongTermLiability'], this.destinationAccounts); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor cccAccpuntOptions for better maintainability
The method needs several improvements:
- Fix typo in method name
- Simplify complex conditional logic
- Reuse account type constants
- Add null check for
destinationAccounts
+ private readonly CREDIT_CARD_ACCOUNT_TYPES = ['CreditCard'] as const;
- cccAccpuntOptions(cccExportType: string): DestinationAttribute[] {
+ private cccAccountOptions(cccExportType: string): DestinationAttribute[] {
+ if (!this.destinationAccounts) {
+ return [];
+ }
if (cccExportType === QBDCorporateCreditCardExpensesObject.CREDIT_CARD_PURCHASE) {
- return this.destinationOptionsWatcher(['CreditCard'], this.destinationAccounts);
+ return this.destinationOptionsWatcher(this.CREDIT_CARD_ACCOUNT_TYPES, this.destinationAccounts);
}
+ const isJournalEntryWithEmployee = cccExportType === QBDCorporateCreditCardExpensesObject.JOURNAL_ENTRY
+ && this.exportSettingsForm.controls.employeeMapping.value === EmployeeFieldMapping.EMPLOYEE
+ && this.exportSettingsForm.controls.nameInJE.value === EmployeeFieldMapping.EMPLOYEE;
- if (cccExportType === QBDCorporateCreditCardExpensesObject.JOURNAL_ENTRY && this.exportSettingsForm.controls.employeeMapping.value === EmployeeFieldMapping.EMPLOYEE && this.exportSettingsForm.controls.nameInJE.value === EmployeeFieldMapping.EMPLOYEE) {
+ if (isJournalEntryWithEmployee) {
- return this.destinationOptionsWatcher(['Bank', 'CreditCard', 'OtherCurrentLiability', 'LongTermLiability'], this.destinationAccounts);
+ return this.destinationOptionsWatcher(this.EMPLOYEE_ACCOUNT_TYPES, this.destinationAccounts);
}
- return this.destinationOptionsWatcher(['Bank', 'AccountsPayable', 'CreditCard', 'OtherCurrentLiability', 'LongTermLiability'], this.destinationAccounts);
+ return this.destinationOptionsWatcher(this.ALL_ACCOUNT_TYPES, this.destinationAccounts);
}Committable suggestion skipped: line range outside the PR's diff.
| reimbursableAccpuntOptions(): DestinationAttribute[] { | ||
| if (this.exportSettingsForm.controls.employeeMapping.value === EmployeeFieldMapping.EMPLOYEE) { | ||
| return this.destinationOptionsWatcher(['Bank', 'CreditCard', 'OtherCurrentLiability', 'LongTermLiability'], this.destinationAccounts); | ||
| } | ||
| return this.destinationOptionsWatcher(['Bank', 'AccountsPayable', 'CreditCard', 'OtherCurrentLiability', 'LongTermLiability'], this.destinationAccounts); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix typo and improve robustness of reimbursableAccpuntOptions method
Several improvements needed:
- Fix typo in method name ("Accpunt" → "Account")
- Add null check for
destinationAccounts - Extract account types as readonly constants
- Consider making method private if only used in template
+ private readonly EMPLOYEE_ACCOUNT_TYPES = ['Bank', 'CreditCard', 'OtherCurrentLiability', 'LongTermLiability'] as const;
+ private readonly ALL_ACCOUNT_TYPES = ['Bank', 'AccountsPayable', 'CreditCard', 'OtherCurrentLiability', 'LongTermLiability'] as const;
- reimbursableAccpuntOptions(): DestinationAttribute[] {
+ private reimbursableAccountOptions(): DestinationAttribute[] {
+ if (!this.destinationAccounts) {
+ return [];
+ }
if (this.exportSettingsForm.controls.employeeMapping.value === EmployeeFieldMapping.EMPLOYEE) {
- return this.destinationOptionsWatcher(['Bank', 'CreditCard', 'OtherCurrentLiability', 'LongTermLiability'], this.destinationAccounts);
+ return this.destinationOptionsWatcher(this.EMPLOYEE_ACCOUNT_TYPES, this.destinationAccounts);
}
- return this.destinationOptionsWatcher(['Bank', 'AccountsPayable', 'CreditCard', 'OtherCurrentLiability', 'LongTermLiability'], this.destinationAccounts);
+ return this.destinationOptionsWatcher(this.ALL_ACCOUNT_TYPES, this.destinationAccounts);
}📝 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.
| reimbursableAccpuntOptions(): DestinationAttribute[] { | |
| if (this.exportSettingsForm.controls.employeeMapping.value === EmployeeFieldMapping.EMPLOYEE) { | |
| return this.destinationOptionsWatcher(['Bank', 'CreditCard', 'OtherCurrentLiability', 'LongTermLiability'], this.destinationAccounts); | |
| } | |
| return this.destinationOptionsWatcher(['Bank', 'AccountsPayable', 'CreditCard', 'OtherCurrentLiability', 'LongTermLiability'], this.destinationAccounts); | |
| } | |
| private readonly EMPLOYEE_ACCOUNT_TYPES = ['Bank', 'CreditCard', 'OtherCurrentLiability', 'LongTermLiability'] as const; | |
| private readonly ALL_ACCOUNT_TYPES = ['Bank', 'AccountsPayable', 'CreditCard', 'OtherCurrentLiability', 'LongTermLiability'] as const; | |
| private reimbursableAccountOptions(): DestinationAttribute[] { | |
| if (!this.destinationAccounts) { | |
| return []; | |
| } | |
| if (this.exportSettingsForm.controls.employeeMapping.value === EmployeeFieldMapping.EMPLOYEE) { | |
| return this.destinationOptionsWatcher(this.EMPLOYEE_ACCOUNT_TYPES, this.destinationAccounts); | |
| } | |
| return this.destinationOptionsWatcher(this.ALL_ACCOUNT_TYPES, this.destinationAccounts); | |
| } |
| @@ -57,7 +57,7 @@ | |||
| <div *ngIf="helperService.isFieldRequired(exportSettingsForm, 'defaultReimbursableAccountsPayableAccountName')"> | |||
| <app-configuration-select-field | |||
| [form]="exportSettingsForm" | |||
| [destinationAttributes]="destinationOptionsWatcher(['AccountsPayable'] , destinationAccounts)" | |||
| [destinationAttributes]="reimbursableAccpuntOptions()" | |||
There was a problem hiding this comment.
💡 Codebase verification
Fix typo in method name and update implementation
The method name reimbursableAccpuntOptions() contains a typo and should be renamed to reimbursableAccountOptions(). The implementation itself is correct as it properly filters destination accounts based on account types for reimbursable expenses.
- Line 60: Rename
reimbursableAccpuntOptions()toreimbursableAccountOptions()
🔗 Analysis chain
Fix typo in method name and verify implementation
The method name contains a typo: reimbursableAccpuntOptions should be reimbursableAccountOptions.
Please verify that the new method correctly handles all account filtering scenarios. Run this script to check the implementation:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find the implementation of reimbursableAccpuntOptions method
# and any related test cases
ast-grep --pattern 'reimbursableAccpuntOptions()' -A 10
rg -l "reimbursableAccpuntOptions" --type spec
Length of output: 253
Script:
#!/bin/bash
# Let's try different approaches to find the implementation:
# 1. Search for the method name with potential typos using ripgrep
rg -i "reimbursable.*options" -A 5
# 2. Search for the component class file to find the implementation
fd -e ts "qbd-direct-export-settings.component.ts$"
# 3. If found, let's examine the component file
if [ -f "src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-export-settings/qbd-direct-export-settings.component.ts" ]; then
cat "src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-export-settings/qbd-direct-export-settings.component.ts"
fi
Length of output: 99845
| defaultCCCAccountPayableLabel: 'Select the Default Credit Account for adding ', | ||
| defaultCCCAccountPayableSubLabel: 'This account will be used for the credit line of the ' |
There was a problem hiding this comment.
💡 Codebase verification
Truncated labels need to be completed
The search results confirm that the labels in c1-contents-config.ts are incomplete and inconsistent with other configurations. The labels appear to be truncated:
defaultCCCAccountPayableLabel: 'Select the Default Credit Account for adding '(ends with "for adding")defaultCCCAccountPayableSubLabel: 'This account will be used for the credit line of the '(ends with "of the")
Both labels are missing their endings, which are necessary for proper user guidance. This appears to be a bug in the configuration.
🔗 Analysis chain
LGTM! Verify label consistency across integrations.
The updated labels provide better clarity for credit account selection. However, let's ensure consistency across other integration configurations.
Inconsistent credit account labels found across integrations
The search results reveal inconsistencies in credit-related labels across different integrations:
- QBD Direct: Uses "Select the Default Credit Account for adding" (the modified label)
- Xero: Uses "The integration will credit the account selected here for corporate credit card expenses exported as journal entries"
- QBO: Similar inconsistency in terminology
Let's verify the exact labels in the QBD Direct section:
Truncated labels found in QBD Direct configuration
The labels appear to be incomplete as they end abruptly:
defaultCCCAccountPayableLabelends with "for adding"defaultCCCAccountPayableSubLabelends with "of the"
This seems to be a potential issue with the modified labels. Let's check if similar labels exist in other integration configurations:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for similar credit account labels across different integration configurations
# to ensure consistent terminology.
# Search for credit account related labels
rg -i "credit.*(account|line).*label" --type ts
# Search for similar subLabel patterns
rg -i "credit.*sublabel" --type ts
Length of output: 10755
Script:
#!/bin/bash
# Get more context around the QBD Direct configuration
rg -B5 -A5 "defaultCCCAccountPayableLabel" src/app/branding/c1-contents-config.ts
Length of output: 1050
Script:
#!/bin/bash
# Search for defaultCCCAccountPayable patterns across all files
rg -B2 -A2 "defaultCCCAccount(Payable)?" --type ts
Length of output: 18089
| }); | ||
| } | ||
|
|
||
| cccAccpuntOptions(): void { |
| } | ||
| } | ||
|
|
||
| categoryOptions(): void { |
Description
feat: Qbd direct je changes
Clickup
https://app.clickup.com/t/86cx6efcg
Summary by CodeRabbit
New Features
Bug Fixes
Documentation