fix: JE fixes #1093
fix: JE fixes #1093
Conversation
WalkthroughThe changes in this pull request focus on enhancing 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 (
|
|
PR description must contain a link to a ClickUp (case-insensitive) |
1 similar comment
|
PR description must contain a link to a ClickUp (case-insensitive) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-export-settings/qbd-direct-export-settings.component.html (1)
Line range hint
251-259: Improve export grouping field configurationA few suggestions to improve this section:
- Consider moving the complex disable condition to a component method for better maintainability
- Add a tooltip or help text explaining why grouping is disabled for CREDIT_CARD_PURCHASE and JOURNAL_ENTRY types
- The
isDisableTextRequiredis set but no disable text is visible in the templateConsider refactoring to:
- [isDisabled]="exportSettingsForm.get('creditCardExportType')?.value === QBDCorporateCreditCardExpensesObject.CREDIT_CARD_PURCHASE || exportSettingsForm.get('creditCardExportType')?.value === QBDCorporateCreditCardExpensesObject.JOURNAL_ENTRY ? true : false" + [isDisabled]="isExportGroupingDisabled()" + [disabledText]="getExportGroupingDisabledText()"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-export-settings/qbd-direct-export-settings.component.html(1 hunks)
🔇 Additional comments (1)
src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-export-settings/qbd-direct-export-settings.component.html (1)
Line range hint 158-170: Review employee mapping visibility logic
The employee mapping section for credit card expenses is only shown when:
- Reimbursable expenses are disabled (
!exportSettingsForm.get('reimbursableExpense')?.value) AND - Credit card export type is Journal Entry
This could lead to missing employee mapping configuration when both reimbursable and credit card expenses are enabled. Consider if this is the intended behavior.
Let's check if this is handled in the component class:
| @@ -194,7 +194,7 @@ | |||
| <div *ngIf="helperService.isFieldRequired(exportSettingsForm, 'defaultCCCAccountsPayableAccountName')"> | |||
| <app-configuration-select-field | |||
| [form]="exportSettingsForm" | |||
| [destinationAttributes]="destinationOptionsWatcher(['AccountsPayable'] , destinationAccounts)" | |||
| [destinationAttributes]="cccAccpuntOptions(exportSettingsForm.get('creditCardExportType')?.value)" | |||
There was a problem hiding this comment.
💡 Codebase verification
Fix typo in function name cccAccpuntOptions
The function name contains a typo in both the component class and template:
src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-export-settings/qbd-direct-export-settings.component.ts: Function declarationcccAccpuntOptionssrc/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-export-settings/qbd-direct-export-settings.component.html: Function usagecccAccpuntOptions
Rename the function to cccAccountOptions to fix the spelling error.
🔗 Analysis chain
Fix typo in function name cccAccpuntOptions
The function name contains a typo: "Accpunt" should be "Account".
Let's verify if this typo exists in the component class:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the misspelled function name in component files
rg "cccAccpuntOptions" --type ts
Length of output: 218
Script:
#!/bin/bash
# Get the implementation of the misspelled function to understand its usage
ast-grep --pattern 'cccAccpuntOptions($$$) {
$$$
}'
Length of output: 55
Script:
#!/bin/bash
# Let's try a different pattern to find the function implementation
rg "cccAccpuntOptions" -A 10 --type ts
Length of output: 2216
|
PR description must contain a link to a ClickUp (case-insensitive) |
1 similar comment
|
PR description must contain a link to a ClickUp (case-insensitive) |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-export-settings/qbd-direct-export-settings.component.ts (3)
Line range hint
111-120: Consider refactoring complex conditional logic for better readabilityThe method handles multiple business cases with nested conditions. Consider extracting the account type arrays into named constants to improve readability and maintainability.
+ private readonly CREDIT_CARD_ACCOUNT_TYPES = ['CreditCard']; + private readonly EMPLOYEE_ACCOUNT_TYPES = ['Bank', 'CreditCard', 'OtherCurrentLiability', 'LongTermLiability']; + private readonly DEFAULT_ACCOUNT_TYPES = ['Bank', 'AccountsPayable', 'CreditCard', 'OtherCurrentLiability', 'LongTermLiability']; cccAccountOptions(cccExportType: string): DestinationAttribute[] { if (cccExportType === QBDCorporateCreditCardExpensesObject.CREDIT_CARD_PURCHASE) { - return this.destinationOptionsWatcher(['CreditCard'], this.destinationAccounts); + return this.destinationOptionsWatcher(this.CREDIT_CARD_ACCOUNT_TYPES, 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(this.EMPLOYEE_ACCOUNT_TYPES, this.destinationAccounts); } - return this.destinationOptionsWatcher(['Bank', 'AccountsPayable', 'CreditCard', 'OtherCurrentLiability', 'LongTermLiability'], this.destinationAccounts); + return this.destinationOptionsWatcher(this.DEFAULT_ACCOUNT_TYPES, this.destinationAccounts); }
Line range hint
122-149: Add subscription cleanup and error handlingThe subscription to
optionSearchUpdateshould be cleaned up on component destruction to prevent memory leaks. Additionally, error handling should be added for the API call.+ private destroy$ = new Subject<void>(); private optionSearchWatcher(): void { this.optionSearchUpdate.pipe( debounceTime(1000), + takeUntil(this.destroy$) ).subscribe((event: ExportSettingOptionSearch) => { let existingOptions: QbdDirectDestinationAttribute[] = []; existingOptions = this.destinationAccounts; let newOptions: QbdDirectDestinationAttribute[]; this.mappingService.getPaginatedDestinationAttributes(event.destinationOptionKey, event.searchTerm).pipe( + catchError((error) => { + this.isOptionSearchInProgress = false; + this.toastService.displayToastMessage(ToastSeverity.ERROR, 'Error fetching options'); + return EMPTY; + }) ).subscribe((response) => { // ... rest of the code }); }); } + ngOnDestroy(): void { + this.destroy$.next(); + this.destroy$.complete(); + }
Line range hint
1-300: Consider splitting component for better maintainabilityThe component handles multiple responsibilities including:
- Export settings form management
- Search functionality
- Multiple watchers
- Complex state management
Consider splitting this into smaller, more focused components or services to improve maintainability and testability.
Suggestions:
- Extract form management logic into a dedicated service
- Create a separate service for handling destination attributes and search
- Consider using NgRx/RxJS state management for complex state handling
- Split the template into smaller, reusable components
Would you like assistance in planning this refactoring?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-export-settings/qbd-direct-export-settings.component.html(2 hunks)src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-export-settings/qbd-direct-export-settings.component.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-export-settings/qbd-direct-export-settings.component.html
| this.exportSettingsForm.controls.creditCardExportType.valueChanges.subscribe((creditCardExportTypeValue) => { | ||
| this.exportSettingsForm.controls.creditCardExportGroup.patchValue(this.creditCardExpenseGroupingFieldOptions[1].value); | ||
| this.exportSettingsForm.controls.creditCardExportGroup.patchValue(QbdDirectExportSettingModel.expenseGroupingFieldOptions()[1].value); | ||
| this.exportSettingsForm.controls.creditCardExportGroup.disable(); | ||
| this.creditCardExpenseGroupingFieldOptions = [QbdDirectExportSettingModel.expenseGroupingFieldOptions()[1]]; | ||
| }); |
There was a problem hiding this comment.
Replace hardcoded index with explicit value reference
Using a hardcoded index [1] for accessing options is fragile and could break if the options array changes. Consider using a constant or finding the option by value.
cccExportTypeWatcher(): void {
this.exportSettingsForm.controls.creditCardExportType.valueChanges.subscribe((creditCardExportTypeValue) => {
- this.exportSettingsForm.controls.creditCardExportGroup.patchValue(QbdDirectExportSettingModel.expenseGroupingFieldOptions()[1].value);
+ const defaultOption = QbdDirectExportSettingModel.expenseGroupingFieldOptions().find(option =>
+ option.value === QbdDirectExpenseGroupBy.EXPENSE
+ );
+ if (!defaultOption) {
+ throw new Error('Default expense grouping option not found');
+ }
+ this.exportSettingsForm.controls.creditCardExportGroup.patchValue(defaultOption.value);
this.exportSettingsForm.controls.creditCardExportGroup.disable();
- this.creditCardExpenseGroupingFieldOptions = [QbdDirectExportSettingModel.expenseGroupingFieldOptions()[1]];
+ this.creditCardExpenseGroupingFieldOptions = [defaultOption];
});
}📝 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.exportSettingsForm.controls.creditCardExportType.valueChanges.subscribe((creditCardExportTypeValue) => { | |
| this.exportSettingsForm.controls.creditCardExportGroup.patchValue(this.creditCardExpenseGroupingFieldOptions[1].value); | |
| this.exportSettingsForm.controls.creditCardExportGroup.patchValue(QbdDirectExportSettingModel.expenseGroupingFieldOptions()[1].value); | |
| this.exportSettingsForm.controls.creditCardExportGroup.disable(); | |
| this.creditCardExpenseGroupingFieldOptions = [QbdDirectExportSettingModel.expenseGroupingFieldOptions()[1]]; | |
| }); | |
| this.exportSettingsForm.controls.creditCardExportType.valueChanges.subscribe((creditCardExportTypeValue) => { | |
| const defaultOption = QbdDirectExportSettingModel.expenseGroupingFieldOptions().find(option => | |
| option.value === QbdDirectExpenseGroupBy.EXPENSE | |
| ); | |
| if (!defaultOption) { | |
| throw new Error('Default expense grouping option not found'); | |
| } | |
| this.exportSettingsForm.controls.creditCardExportGroup.patchValue(defaultOption.value); | |
| this.exportSettingsForm.controls.creditCardExportGroup.disable(); | |
| this.creditCardExpenseGroupingFieldOptions = [defaultOption]; | |
| }); |
DhaaraniCIT
added
deploy
|
PR description must contain a link to a ClickUp (case-insensitive) |
1 similar comment
|
PR description must contain a link to a ClickUp (case-insensitive) |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
src/app/core/models/qbd-direct/qbd-direct-configuration/qbd-direct-advanced-settings.model.ts (1)
55-55: Address static method references and simplify null checksThe current implementation has two issues:
- Uses
thisin static context (flagged by static analysis)- Has complex null checking logic
Consider this cleaner implementation:
- topMemoStructure: new FormControl(advancedSettings?.top_level_memo_structure && advancedSettings?.top_level_memo_structure.length > 0 ? this.topMemoExpenseKeyNameConversion(advancedSettings?.top_level_memo_structure) : this.defaultTopMemoOptions(), Validators.required), + topMemoStructure: new FormControl( + QbdDirectAdvancedSettingsModel.topMemoExpenseKeyNameConversion(advancedSettings?.top_level_memo_structure) || + QbdDirectAdvancedSettingsModel.defaultTopMemoOptions(), + Validators.required + ),🧰 Tools
🪛 Biome (1.9.4)
[error] 55-55: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 55-55: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
src/app/core/services/common/dashboard.service.ts (1)
61-61: Consider simplifying the complex ternary expression.While the change to use
export_log__id__inis correct and consistent with the model update, the nested ternary operator is becoming hard to read.Consider refactoring to improve readability:
-const expenseKey = appName === AppName.INTACCT ? 'expense_group_ids' : appName === AppName.QBD_DIRECT ? 'export_log__id__in' : 'expense_group_id__in'; +const getExpenseKey = (appName?: AppName): string => { + switch (appName) { + case AppName.INTACCT: + return 'expense_group_ids'; + case AppName.QBD_DIRECT: + return 'export_log__id__in'; + default: + return 'expense_group_id__in'; + } +}; +const expenseKey = getExpenseKey(appName);This approach:
- Improves readability
- Makes it easier to add new cases
- Makes the logic more maintainable
src/app/core/models/qbd-direct/qbd-direct-configuration/qbd-direct-export-settings.model.ts (1)
Line range hint
177-203: Consider broader implications of validation changesThe validation rule changes for employeeMapping could affect:
- UI components that rely on these validation rules
- Error messages and user feedback
- Existing data that might not meet the new validation requirements
Please ensure that:
- Related components are updated to handle the new validation rules
- Error messages clearly indicate when employeeMapping is required
- A data migration plan is in place if needed
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
src/app/core/models/common/helper.model.ts(1 hunks)src/app/core/models/db/task-log.model.ts(1 hunks)src/app/core/models/qbd-direct/qbd-direct-configuration/qbd-direct-advanced-settings.model.ts(2 hunks)src/app/core/models/qbd-direct/qbd-direct-configuration/qbd-direct-export-settings.model.ts(2 hunks)src/app/core/services/common/dashboard.service.ts(1 hunks)src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-export-settings/qbd-direct-export-settings.component.html(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-export-settings/qbd-direct-export-settings.component.html
🧰 Additional context used
🪛 Biome (1.9.4)
src/app/core/models/qbd-direct/qbd-direct-configuration/qbd-direct-advanced-settings.model.ts
[error] 55-55: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 55-55: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
🔇 Additional comments (2)
src/app/core/models/db/task-log.model.ts (1)
26-26: LGTM! Consistent with REST API filtering conventions.
The rename from export_log__id to export_log__id__in better reflects that this parameter accepts multiple IDs for filtering, following standard REST API naming conventions.
src/app/core/models/qbd-direct/qbd-direct-configuration/qbd-direct-export-settings.model.ts (1)
187-187: Verify validation behavior for employeeMapping
The validation rules for employeeMapping have been updated to be more specific to export types, which is good. However, please verify that:
- Removing employeeMapping from reimbursableExpense array doesn't create validation gaps
- These validation rules work correctly with the getMandatoryField logic
Let's verify the validation behavior:
Also applies to: 195-196, 203-203
✅ Verification successful
Let's gather more specific information about the validation rules and their usage.
Let's try a different approach to gather the validation logic:
After analyzing the file content, I can now provide a final response:
Validation rules for employeeMapping are correctly structured
The changes to employeeMapping validation are intentional and properly implemented:
- The removal from reimbursableExpense array is correct because employeeMapping is now handled through the exportModuleRule structure, which provides more granular control
- The getMandatoryField logic aligns with the validation rules:
- For BILL type: employeeMapping is required (via exportModuleRule)
- For JOURNAL_ENTRY type: employeeMapping is required (via exportModuleRule)
- For credit card expenses: employeeMapping is required only for JOURNAL_ENTRY type (via both getMandatoryField and exportModuleRule)
The validation structure ensures there are no gaps in validation coverage and maintains consistency between the validation rules and mandatory field logic.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for tests or usages of these validation rules
rg -l "getValidators|getMandatoryField.*employeeMapping" --type ts
# Search for any related validation error handling
rg "employeeMapping.*error" --type ts
Length of output: 1359
Script:
#!/bin/bash
# Check the implementation of getValidators and getMandatoryField in the file
ast-grep --pattern 'getValidators() {
$$$
}'
# Look for employeeMapping related validation tests
rg -A 5 "employeeMapping.*test" --type ts
# Check the actual implementation in the component
rg -A 10 "employeeMapping.*validate" --type ts
# Look for the specific validation logic around reimbursableExpense
ast-grep --pattern 'reimbursableExpense: [$_]'
Length of output: 190
Script:
#!/bin/bash
# Get the content of the file we're reviewing to see the full context
cat src/app/core/models/qbd-direct/qbd-direct-configuration/qbd-direct-export-settings.model.ts
# Search for any references to employeeMapping in validation contexts
rg -B 2 -A 2 "employeeMapping.*validation|validate.*employeeMapping" --type ts
# Look for any tests related to these export settings
fd "export-settings.spec" --type ts --exec cat {}
Length of output: 15114
| static topMemoExpenseKeyNameConversion(keys: string[]): string[] { | ||
| keys.forEach((key: string, index: number) => { | ||
| if (key === 'expense_key') { | ||
| keys[index] = 'Expense/Report ID'; | ||
| } | ||
| }); | ||
| return keys; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve method implementation to follow best practices
The current implementation has potential issues:
- Mutates the input array directly
- Lacks input validation
- Has unclear return semantics (both mutates and returns)
Consider this safer implementation:
- static topMemoExpenseKeyNameConversion(keys: string[]): string[] {
- keys.forEach((key: string, index: number) => {
- if (key === 'expense_key') {
- keys[index] = 'Expense/Report ID';
- }
- });
- return keys;
- }
+ static topMemoExpenseKeyNameConversion(keys: string[] | null | undefined): string[] {
+ if (!keys?.length) {
+ return [];
+ }
+ return keys.map((key: string) =>
+ key === 'expense_key' ? 'Expense/Report ID' : key
+ );
+ }📝 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.
| static topMemoExpenseKeyNameConversion(keys: string[]): string[] { | |
| keys.forEach((key: string, index: number) => { | |
| if (key === 'expense_key') { | |
| keys[index] = 'Expense/Report ID'; | |
| } | |
| }); | |
| return keys; | |
| } | |
| static topMemoExpenseKeyNameConversion(keys: string[] | null | undefined): string[] { | |
| if (!keys?.length) { | |
| return []; | |
| } | |
| return keys.map((key: string) => | |
| key === 'expense_key' ? 'Expense/Report ID' : key | |
| ); | |
| } |
| @@ -14,7 +14,7 @@ export class HelperUtility { | |||
| form.controls[controllerName].clearValidators(); | |||
|
|
|||
| // Temporary hack: Don't reset value for fields that are used in reimbursable + ccc section | |||
| if (controllerName !== 'accountsPayable' && controllerName !== 'defaultCCCAccount' && controllerName !== 'bankAccount' && controllerName !== 'creditCardAccount' && controllerName !== 'defaultCreditCardVendor' && controllerName !== 'nameInJournalEntry' && controllerName !== 'exportScheduleFrequency') { | |||
| if (controllerName !== 'accountsPayable' && controllerName !== 'defaultCCCAccount' && controllerName !== 'bankAccount' && controllerName !== 'creditCardAccount' && controllerName !== 'defaultCreditCardVendor' && controllerName !== 'nameInJournalEntry' && controllerName !== 'exportScheduleFrequency' && controllerName !== 'employeeMapping') { | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring the growing list of exceptions.
The current approach of listing specific field names as exceptions is becoming unwieldy and harder to maintain. The comment "Temporary hack" also suggests this needs improvement.
Consider refactoring to a more maintainable solution:
-if (controllerName !== 'accountsPayable' && controllerName !== 'defaultCCCAccount' && controllerName !== 'bankAccount' && controllerName !== 'creditCardAccount' && controllerName !== 'defaultCreditCardVendor' && controllerName !== 'nameInJournalEntry' && controllerName !== 'exportScheduleFrequency' && controllerName !== 'employeeMapping') {
+const PRESERVE_VALUE_FIELDS = new Set([
+ 'accountsPayable',
+ 'defaultCCCAccount',
+ 'bankAccount',
+ 'creditCardAccount',
+ 'defaultCreditCardVendor',
+ 'nameInJournalEntry',
+ 'exportScheduleFrequency',
+ 'employeeMapping'
+]);
+
+if (!PRESERVE_VALUE_FIELDS.has(controllerName)) {This approach:
- Makes the list of exceptions more maintainable
- Improves readability
- Provides better performance for lookups
- Makes it easier to add/remove fields
Committable suggestion skipped: line range outside the PR's diff.
| case 'employeeMapping': | ||
| return !form.get('reimbursableExportType')?.value && form.get('creditCardExportType')?.value && form.get('creditCardExportType')?.value === QBDCorporateCreditCardExpensesObject.JOURNAL_ENTRY; |
There was a problem hiding this comment.
Review the employeeMapping mandatory field logic
The current condition for making employeeMapping mandatory seems inconsistent. It only requires employeeMapping when reimbursableExportType is NOT set, but the validation rules later show that employeeMapping is required for both BILL and JOURNAL_ENTRY types. This could lead to validation inconsistencies.
Consider updating the condition to:
case 'employeeMapping':
- return !form.get('reimbursableExportType')?.value && form.get('creditCardExportType')?.value && form.get('creditCardExportType')?.value === QBDCorporateCreditCardExpensesObject.JOURNAL_ENTRY;
+ return (form.get('reimbursableExportType')?.value === QBDReimbursableExpensesObject.BILL ||
+ form.get('reimbursableExportType')?.value === QBDReimbursableExpensesObject.JOURNAL_ENTRY) ||
+ (form.get('creditCardExportType')?.value === QBDCorporateCreditCardExpensesObject.JOURNAL_ENTRY);Committable suggestion skipped: line range outside the PR's diff.
|
PR description must contain a link to a ClickUp (case-insensitive) |
|
|
PR description must contain a link to a ClickUp (case-insensitive) |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
src/app/integrations/qbd-direct/qbd-direct-main/qbd-direct-dashboard/qbd-direct-dashboard.component.ts (1)
Line range hint
143-157: Enhance error handling for API callsThe
setupPagemethod only handles errors forgetAccountingExportSummary$. Consider adding error handling for other API calls to ensure graceful degradation.Example implementation:
private setupPage(): void { forkJoin([ this.getExportErrors$.pipe(catchError(() => of({ results: [] }))), this.getAccountingExportSummary$.pipe(catchError(() => of(null))), this.dashboardService.getAllTasks(...).pipe(catchError(() => of({ results: [] }))), this.dashboardService.getExportableAccountingExportIds('v2').pipe(catchError(() => of({ exportable_export_log_ids: [] }))), this.QbdDirectExportSettingsService.getQbdExportSettings().pipe(catchError(() => of({}))), this.importSettingService.getImportSettings().pipe(catchError(() => of({ import_settings: { chart_of_accounts: [] } }))) ]).subscribe(...) }src/app/shared/components/dashboard/dashboard-error-section/dashboard-error-section.component.ts (2)
55-56: Add documentation and consider default value.The
chartOfAccountsinput property would benefit from:
- JSDoc documentation explaining its purpose and expected values
- A default empty array value to prevent potential undefined errors
+ /** + * List of account types used to filter QBD Direct destination options + */ - @Input() chartOfAccounts: string[]; + @Input() chartOfAccounts: string[] = [];
152-152: Improve type safety and readability.The current implementation could be enhanced:
- Split the long line for better readability
- Add type guard to ensure safe type casting
- this.destinationOptions = this.appName !== AppName.QBD_DIRECT ? groupedDestinationResponse.ACCOUNT : this.destinationOptionsWatcher( this.chartOfAccounts, groupedDestinationResponse.ACCOUNT as QbdDirectDestinationAttribute[]); + const accounts = groupedDestinationResponse.ACCOUNT; + this.destinationOptions = this.appName !== AppName.QBD_DIRECT + ? accounts + : this.filterDestinationOptionsByAccountType( + this.chartOfAccounts, + this.isQbdDirectDestinationAttribute(accounts) ? accounts : [] + ); + + private isQbdDirectDestinationAttribute(accounts: any[]): accounts is QbdDirectDestinationAttribute[] { + return accounts?.length > 0 && accounts.every(account => + account.detail && typeof account.detail.account_type === 'string' + ); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
src/app/core/models/db/destination-attribute.model.ts(2 hunks)src/app/integrations/qbd-direct/qbd-direct-main/qbd-direct-dashboard/qbd-direct-dashboard.component.html(1 hunks)src/app/integrations/qbd-direct/qbd-direct-main/qbd-direct-dashboard/qbd-direct-dashboard.component.ts(2 hunks)src/app/shared/components/dashboard/dashboard-error-section/dashboard-error-section.component.ts(4 hunks)
🔇 Additional comments (5)
src/app/integrations/qbd-direct/qbd-direct-main/qbd-direct-dashboard/qbd-direct-dashboard.component.html (1)
46-46: LGTM: Property binding for chartOfAccounts
The addition of the [chartOfAccounts] input property to the error section component is properly implemented and follows Angular's property binding conventions.
src/app/core/models/db/destination-attribute.model.ts (1)
40-40: LGTM: Type extension for QBD Direct integration
The addition of QbdDirectDestinationAttribute to the union type is consistent with the existing pattern for other integrations.
src/app/integrations/qbd-direct/qbd-direct-main/qbd-direct-dashboard/qbd-direct-dashboard.component.ts (1)
81-82: LGTM: Chart of accounts property declaration
The property is correctly typed as string array.
src/app/shared/components/dashboard/dashboard-error-section/dashboard-error-section.component.ts (2)
16-16: LGTM!
The import statement for QbdDirectDestinationAttribute is correctly placed and follows project conventions.
Line range hint 1-256: Verify error handling consistency across different app types.
Please ensure that these QBD Direct specific changes don't affect error handling for other app types. Consider adding tests to verify:
- Error handling behavior remains unchanged for non-QBD Direct apps
- New QBD Direct filtering logic works correctly with various account types
- Component maintains backward compatibility
✅ Verification successful
Based on the gathered evidence, I can now provide the final response:
QBD Direct changes maintain error handling consistency across app types
The changes for QBD Direct integration are properly isolated and maintain backward compatibility:
- Error model implementation remains app-agnostic, handling all error types uniformly through
formatErrorsandgetErroredMappings - QBD Direct specific logic is properly guarded with
appName !== AppName.QBD_DIRECTcondition - Core error handling for category and employee mappings remains unchanged
- The component maintains consistent error tracking and resolution across all app types
However, test coverage needs improvement as the spec file only contains a basic creation test (marked with xdescribe). Consider adding tests for:
- QBD Direct specific destination option filtering
- Error handling across different app types
- Mapping resolution scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test files and error handling patterns
echo "Searching for related test files..."
fd -e spec.ts dashboard-error-section
echo "Checking error handling patterns across app types..."
rg -A 5 "handleResolvedMappingStat|showErrorDialog" --type ts
echo "Checking QBD Direct specific error handling..."
rg -A 5 "QBD_DIRECT.*error" --type ts
Length of output: 2381
Script:
#!/bin/bash
# Let's check the test file to verify error handling across different app types
echo "Checking test coverage for error handling..."
cat src/app/shared/components/dashboard/dashboard-error-section/dashboard-error-section.component.spec.ts
echo "\nChecking QBD Direct specific implementations..."
ast-grep --pattern 'if ($condition) {
$$$
this.destinationOptions = this.appName !== AppName.QBD_DIRECT ? $_ : this.destinationOptionsWatcher($$$)
$$$
}'
echo "\nChecking app type handling in the component..."
rg -B 2 -A 2 "AppName\." src/app/shared/components/dashboard/dashboard-error-section/dashboard-error-section.component.ts
Length of output: 2089
Script:
#!/bin/bash
# Let's check the error model and handling implementation
echo "Checking ErrorModel implementation..."
rg -A 10 "class ErrorModel" --type ts
echo "\nChecking error handling for different app types..."
rg -A 5 "formatErrors|getErroredMappings" --type ts
echo "\nChecking destination attribute handling..."
ast-grep --pattern 'getGroupedDestinationAttributes($$$) {
$$$
}'
Length of output: 5489
| @@ -1,4 +1,5 @@ | |||
| import { BusinessCentralDestinationAttributes } from "../business-central/db/business-central-destination-attribute.model"; | |||
| import { QbdDirectDestinationAttribute } from "../qbd-direct/db/qbd-direct-destination-attribuite.model"; | |||
There was a problem hiding this comment.
Fix typo in import path
The import path contains a spelling error: "attribuite" should be "attribute".
-import { QbdDirectDestinationAttribute } from "../qbd-direct/db/qbd-direct-destination-attribuite.model";
+import { QbdDirectDestinationAttribute } from "../qbd-direct/db/qbd-direct-destination-attribute.model";📝 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.
| import { QbdDirectDestinationAttribute } from "../qbd-direct/db/qbd-direct-destination-attribuite.model"; | |
| import { QbdDirectDestinationAttribute } from "../qbd-direct/db/qbd-direct-destination-attribute.model"; |
| this.chartOfAccounts = responses[5].import_settings.chart_of_accounts; | ||
|
|
There was a problem hiding this comment.
Add null safety check for import_settings
The code assumes import_settings exists in the response. Add a null check to prevent potential runtime errors.
- this.chartOfAccounts = responses[5].import_settings.chart_of_accounts;
+ this.chartOfAccounts = responses[5].import_settings?.chart_of_accounts ?? [];📝 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.chartOfAccounts = responses[5].import_settings.chart_of_accounts; | |
| this.chartOfAccounts = responses[5].import_settings?.chart_of_accounts ?? []; |
| destinationOptionsWatcher(detailAccountType: string[], destinationOptions: QbdDirectDestinationAttribute[]): DestinationAttribute[] { | ||
| return destinationOptions.filter((account: QbdDirectDestinationAttribute) => detailAccountType.includes(account.detail.account_type)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving method name and robustness.
The current implementation could be enhanced for better clarity and safety:
- The method name
destinationOptionsWatchersuggests it's watching for changes, but it's actually filtering options. Consider renaming tofilterDestinationOptionsByAccountType. - Add null checks to prevent runtime errors.
- destinationOptionsWatcher(detailAccountType: string[], destinationOptions: QbdDirectDestinationAttribute[]): DestinationAttribute[] {
- return destinationOptions.filter((account: QbdDirectDestinationAttribute) => detailAccountType.includes(account.detail.account_type));
+ private filterDestinationOptionsByAccountType(detailAccountType: string[], destinationOptions: QbdDirectDestinationAttribute[]): DestinationAttribute[] {
+ if (!detailAccountType?.length || !destinationOptions?.length) {
+ return [];
+ }
+ return destinationOptions.filter((account: QbdDirectDestinationAttribute) =>
+ account.detail?.account_type && detailAccountType.includes(account.detail.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.
| destinationOptionsWatcher(detailAccountType: string[], destinationOptions: QbdDirectDestinationAttribute[]): DestinationAttribute[] { | |
| return destinationOptions.filter((account: QbdDirectDestinationAttribute) => detailAccountType.includes(account.detail.account_type)); | |
| } | |
| private filterDestinationOptionsByAccountType(detailAccountType: string[], destinationOptions: QbdDirectDestinationAttribute[]): DestinationAttribute[] { | |
| if (!detailAccountType?.length || !destinationOptions?.length) { | |
| return []; | |
| } | |
| return destinationOptions.filter((account: QbdDirectDestinationAttribute) => | |
| account.detail?.account_type && detailAccountType.includes(account.detail.account_type) | |
| ); | |
| } |
Description
fix: JE fixes
Clickup
Please add link here
Summary by CodeRabbit
New Features
employeeMappingfield.Bug Fixes