feat: add multiselect to choose types of accounts to import #1099
Conversation
WalkthroughThe changes in this pull request enhance the Business Central import settings by modifying the Changes
Possibly related PRs
Suggested reviewers
Poem
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: 1
🧹 Outside diff range and nitpick comments (6)
src/app/core/models/business-central/business-central-configuration/business-central-import-settings.model.ts (2)
29-29: Consider extracting default value to a constantThe default value
['Expense']should be extracted to a named constant for better maintainability and reuse.+private static readonly DEFAULT_CHART_OF_ACCOUNTS = ['Expense']; static mapAPIResponseToFormGroup(importSettings: BusinessCentralImportSettingsGet | null, businessCentralFields: IntegrationField[]): FormGroup { const expenseFieldsArray = importSettings?.mapping_settings ? this.constructFormArray(importSettings.mapping_settings, businessCentralFields) : [] ; return new FormGroup({ importCategories: new FormControl(importSettings?.import_settings?.import_categories ?? false), - chartOfAccountTypes: new FormControl(importSettings?.import_settings.charts_of_accounts ? importSettings?.import_settings.charts_of_accounts : ['Expense']), + chartOfAccountTypes: new FormControl(importSettings?.import_settings.charts_of_accounts ?? this.DEFAULT_CHART_OF_ACCOUNTS),
48-50: Consider making chart types configurableThe hard-coded list of chart account types could be problematic for maintenance. Consider:
- Moving this to a configuration file
- Fetching supported types from the backend
- Adding validation against supported types
This would make the system more maintainable and prevent potential mismatches with backend capabilities.
src/app/integrations/business-central/business-central-shared/business-central.fixture.ts (1)
51-52: Consider adding more test casesWhile the basic case is covered, consider adding test fixtures for:
- Multiple selected account types
- Empty array case
- Edge cases with invalid values
This would help ensure robust testing of the new functionality.
src/app/integrations/business-central/business-central-shared/business-central-import-settings/business-central-import-settings.component.html (1)
34-41: Enhance accessibility and error handling for the multi-select componentThe multi-select implementation could benefit from improved accessibility and error handling:
- <p-multiSelect [optionDisabled]="'Expense'" [placeholder]="'Select Chart of Accounts'" [options]="chartOfAccountTypesList" styleClass="tw-z-2 tw-py-8-px tw-px-12-px" [formControlName]="'chartOfAccountTypes'"> + <p-multiSelect + [optionDisabled]="'Expense'" + [placeholder]="'Select Chart of Accounts'" + [options]="chartOfAccountTypesList" + styleClass="tw-z-2 tw-py-8-px tw-px-12-px" + [formControlName]="'chartOfAccountTypes'" + ariaLabel="Select chart of accounts types" + [showClear]="true" + [maxSelectedLabels]="3" + [loading]="isChartOfAccountTypesLoading">src/app/integrations/business-central/business-central-shared/business-central-import-settings/business-central-import-settings.component.ts (2)
53-53: Add loading state management for chart of account typesThe component should manage a loading state for the chart of account types to provide better user feedback.
+ isChartOfAccountTypesLoading: boolean = false; readonly chartOfAccountTypesList: string[] = BusinessCentralImportSettingsModel.getChartOfAccountTypesList();
73-73: Consider keeping helper service privateThe helper service is made public solely for template access. Consider keeping it private and exposing only the required functionality through component methods.
- public helper: HelperService, + private helper: HelperService, + // Add public method to access required helper functionality + public sentenceCaseConversion(text: string): string { + return this.helper.sentenseCaseConversion(text); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
src/app/core/models/business-central/business-central-configuration/business-central-import-settings.model.ts(3 hunks)src/app/integrations/business-central/business-central-shared/business-central-import-settings/business-central-import-settings.component.html(1 hunks)src/app/integrations/business-central/business-central-shared/business-central-import-settings/business-central-import-settings.component.ts(2 hunks)src/app/integrations/business-central/business-central-shared/business-central-shared.module.ts(2 hunks)src/app/integrations/business-central/business-central-shared/business-central.fixture.ts(1 hunks)
🔇 Additional comments (5)
src/app/integrations/business-central/business-central-shared/business-central-shared.module.ts (1)
7-7: LGTM: MultiSelect module integration
The addition of PrimeNG's MultiSelectModule is appropriate for implementing the chart of accounts selector functionality.
Also applies to: 17-17
src/app/core/models/business-central/business-central-configuration/business-central-import-settings.model.ts (2)
8-9: LGTM: Type definition update
The addition of charts_of_accounts to the import settings interface is well-structured and properly typed.
41-42: LGTM: Payload construction
The payload construction correctly includes the new chart of accounts field.
src/app/integrations/business-central/business-central-shared/business-central-import-settings/business-central-import-settings.component.html (1)
25-44: Verify error handling for required selection
The multi-select section should indicate if a selection is required and display validation errors.
src/app/integrations/business-central/business-central-shared/business-central-import-settings/business-central-import-settings.component.ts (1)
53-54: Verify the completeness of chart of account types list
Ensure that the static list includes all possible account types from Business Central.
✅ Verification successful
Let me analyze the results. I can see that the getChartOfAccountTypesList() method for Business Central is implemented in src/app/core/models/business-central/business-central-configuration/business-central-import-settings.model.ts and returns the following account types:
- Expense
- Assets
- Income
- Equity
- Liabilities
- Others
- Cost of Goods Sold
Let me verify if this list matches with Business Central's account types by searching for any related documentation or schema definitions.
Let me search for any Business Central API models or interfaces that might define the account types.
Let me search for any Business Central account type definitions in the API models.
Let me check the content of these enums to see if they define the account types.
Based on the analysis of the codebase, I can now provide a final response:
The chart of account types list is complete and accurate
The implementation in BusinessCentralImportSettingsModel.getChartOfAccountTypesList() returns all the standard account types used in Business Central:
- Expense
- Assets
- Income
- Equity
- Liabilities
- Others
- Cost of Goods Sold
These account types align with the standard accounting categories in Business Central and cover all the main types of accounts needed for financial reporting and accounting operations. The list is comprehensive and includes both balance sheet accounts (Assets, Liabilities, Equity) and income statement accounts (Income, Expense, Cost of Goods Sold), plus a catch-all 'Others' category for special cases.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check the implementation of getChartOfAccountTypesList
rg -A 10 "getChartOfAccountTypesList" src/
Length of output: 16438
Script:
#!/bin/bash
# Search for Business Central account types in the codebase
rg -i "account.*type" --type ts src/app/core/models/business-central/
Length of output: 701
Script:
#!/bin/bash
# Search for Business Central API interfaces and type definitions
rg -A 5 "interface.*Business.*Central.*" src/
rg -A 5 "type.*Business.*Central.*" src/
Length of output: 19370
Script:
#!/bin/bash
# Search for Business Central account types in models and API files
rg -i "account.*type.*=.*" src/app/core/models/business-central/
rg -i "enum.*business.*central" src/
Length of output: 511
Script:
#!/bin/bash
# Check the content of Business Central related enums
rg -A 10 "enum BusinessCentral" src/app/core/models/enum/enum.model.ts
Length of output: 1658
...entral-shared/business-central-import-settings/business-central-import-settings.component.ts
Show resolved
Hide resolved
* fix: update default bank a/c options in BC We now show `ACCOUNT` as well as `BANK_ACCOUNT` attributes as options for default bank accounts. Only `ACCOUNT`s of category 'Assets' or 'Liabilities' are shown * fix: after autocomplete search, reset the in progress state of the field
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
src/app/core/services/common/mapping.service.ts (3)
175-175: Fix typo in type definition.There's a typo in the interface property name:
detail__accout_type__inshould bedetail__account_type__in.- const params: {limit: number, offset: number, attribute_type: string, active?: boolean, value__icontains?: string, value?: string, display_name__in?: string, detail__accout_type__in?: string[], detail__category__in?: string[]} = { + const params: {limit: number, offset: number, attribute_type: string, active?: boolean, value__icontains?: string, value?: string, display_name__in?: string, detail__account_type__in?: string[], detail__category__in?: string[]} = {
198-201: Consider adding input validation for categories.While the implementation is correct, consider adding validation to ensure the categories array is not empty when provided. This would prevent unnecessary API calls with empty category filters.
if (categories) { + if (categories.length > 0) { params.detail__category__in = categories; + } }
Based on the gathered information, I can now provide the final response:
Test coverage needed for new filtering functionality
The mapping service lacks proper test coverage:
- The
mapping.service.spec.tsonly contains a basic creation test- No test cases exist for
getPaginatedDestinationAttributesmethod and its filtering capabilities- The API endpoint
/workspaces/{id}/mappings/paginated_destination_attributes/is used in QBO fixtures with similar filtering parameters, suggesting the endpoint supports such filteringPlease add test cases to cover:
- Basic filtering with categories
- Multiple category filtering
- Interaction with other filter parameters
🔗 Analysis chain
Line range hint
173-201: Verify API endpoint compatibility and test coverage.Please ensure:
- The API endpoint
/workspaces/{id}/mappings/paginated_destination_attributes/supports the newdetail__category__infilter parameter.- Unit tests are updated to cover the new categories filtering functionality.
Let me gather more information about the test coverage by searching for spec files specifically.
Let me check the specific content of the mapping service spec file to see if there are any tests for the getPaginatedDestinationAttributes method.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for test coverage and API endpoint usage # Check for test file existence and content echo "Checking for test coverage..." rg -l "getPaginatedDestinationAttributes.*categories" --type ts "spec.ts$" # Check for API endpoint usage with the new parameter echo "Checking API endpoint usage..." rg -A 5 "detail__category__in" --type tsLength of output: 1392
Script:
#!/bin/bash # Search for mapping service spec file echo "Searching for mapping service spec file..." fd "mapping.service.spec.ts" # If found, check its content for test cases echo "Checking test content..." rg -A 10 "describe.*MappingService" --type tsLength of output: 6362
Script:
#!/bin/bash # Check content of mapping.service.spec.ts echo "Checking mapping service spec content..." cat src/app/core/services/common/mapping.service.spec.ts # Check if the API endpoint is used elsewhere in the codebase echo "Checking API endpoint usage..." rg -A 2 "mappings/paginated_destination_attributes" --type tsLength of output: 1579
src/app/integrations/business-central/business-central-shared/business-central-export-settings/business-central-export-settings.component.ts (2)
173-191: Consider optimizing duplicate checking in 'optionSearchWatcher' methodIn the
optionSearchWatchermethod, when handlingREIMBURSABLE_BANK_ACCOUNT, the duplicate check usesfindinside aforEachloop, resulting in O(n²) time complexity for large datasets.Consider using a
SetorMapto storedestination_ids for faster lookup, improving performance for larger data sets.Possible code change:
+ const existingIds = new Set(this.reimbursableBankOptions.map(option => option.destination_id)); newOptions.forEach((newOption) => { - if (!this.reimbursableBankOptions.find((existingOption) => existingOption.destination_id === newOption.destination_id)) { + if (!existingIds.has(newOption.destination_id)) { this.reimbursableBankOptions.push(newOption); + existingIds.add(newOption.destination_id); } });
209-214: Optimize duplicate checking when updating 'existingOptions'The duplicate check in updating
existingOptionsusesfindinside aforEachloop, which can be inefficient for larger datasets.Use a
Setto store existingdestination_ids for faster lookup.Possible code change:
+ const existingIds = new Set(existingOptions.map(option => option.destination_id)); response.results.forEach((option) => { - if (!existingOptions.find((existingOption) => existingOption.destination_id === option.destination_id)) { + if (!existingIds.has(option.destination_id)) { existingOptions.push(option); + existingIds.add(option.destination_id); } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
src/app/core/models/enum/enum.model.ts(1 hunks)src/app/core/services/common/mapping.service.ts(2 hunks)src/app/integrations/business-central/business-central-shared/business-central-export-settings/business-central-export-settings.component.html(1 hunks)src/app/integrations/business-central/business-central-shared/business-central-export-settings/business-central-export-settings.component.ts(4 hunks)
🔇 Additional comments (7)
src/app/core/services/common/mapping.service.ts (1)
173-173: LGTM! Method signature change maintains backward compatibility.
The addition of the optional categories parameter is well-placed and follows TypeScript conventions.
src/app/integrations/business-central/business-central-shared/business-central-export-settings/business-central-export-settings.component.ts (4)
34-35: Addition of 'reimbursableBankOptions' property is appropriate
The new property reimbursableBankOptions: DestinationAttribute[]; is correctly declared and will hold the destination attributes for reimbursable bank accounts.
217-225: LGTM
The code correctly updates and sorts the bankOptions and vendorOptions based on the search results.
269-276: LGTM
The reimbursableBankAccountAttributes observables are correctly defined to fetch bank accounts and accounts with categories 'Assets' and 'Liabilities'.
304-306: LGTM
The reimbursableBankOptions are properly initialized by combining and sorting the results from reimbursableBankAccounts and reimbursableAccounts.
src/app/integrations/business-central/business-central-shared/business-central-export-settings/business-central-export-settings.component.html (1)
94-95: Updated binding of 'destinationAttributes' and 'destinationOptionKey'
The destinationAttributes now correctly bind to reimbursableBankOptions, and the destinationOptionKey is updated to BCExportSettingDestinationOptionKey.REIMBURSABLE_BANK_ACCOUNT, aligning with the new handling of reimbursable bank accounts.
src/app/core/models/enum/enum.model.ts (1)
705-706: Addition of 'REIMBURSABLE_BANK_ACCOUNT' to Enum
The new enum value REIMBURSABLE_BANK_ACCOUNT = 'REIMBURSABLE_BANK_ACCOUNT' is correctly added to BCExportSettingDestinationOptionKey.
...entral-shared/business-central-export-settings/business-central-export-settings.component.ts
Show resolved
Hide resolved
Clickup
https://app.clickup.com/t/86cx9j8dn
Summary by CodeRabbit
New Features
Bug Fixes
Chores
MultiSelectModulefromprimengto support new dropdown functionality.