fix: update default bank a/c options in BC #1098
Conversation
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
WalkthroughThe changes in this pull request involve modifications to several components related to the Business Central export settings. A new enum member Changes
Possibly related PRs
Suggested labels
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: 4
🧹 Outside diff range and nitpick comments (4)
src/app/integrations/business-central/business-central-shared/business-central-export-settings/business-central-export-settings.component.ts (1)
183-190: Consider refactoring the option update logic to avoid duplicationThe code for updating and sorting options is similar in both the
ifandelseblocks. Refactoring this logic into a separate method can enhance maintainability and reduce code duplication.src/app/core/services/common/mapping.service.ts (1)
Line range hint
173-201: Update JSDoc comments to reflect the new parameterThe
getPaginatedDestinationAttributesmethod now includes a new parametercategories. Update the method's documentation to include this parameter for clarity.src/app/integrations/business-central/business-central-shared/business-central-export-settings/business-central-export-settings.component.html (1)
94-95: Update the form control name for clarityThe form control name
'defaultBankName'might be misleading in the context of reimbursable expenses. Consider renaming it to'reimbursableDefaultBankName'to improve clarity.Apply this diff to rename the form control:
[formControllerName]="'defaultBankName'"> + [formControllerName]="'reimbursableDefaultBankName'">Also, update the corresponding TypeScript code to reflect this change.
src/app/core/models/enum/enum.model.ts (1)
705-706: Alphabetical ordering of enum membersFor better readability and maintainability, consider ordering the enum members alphabetically.
Apply this diff to reorder the enum members:
export enum BCExportSettingDestinationOptionKey { + ACCOUNT = 'ACCOUNT', REIMBURSABLE_BANK_ACCOUNT = 'REIMBURSABLE_BANK_ACCOUNT', - ACCOUNT = 'ACCOUNT', VENDOR = 'VENDOR', }
📜 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 (2)
src/app/integrations/business-central/business-central-shared/business-central-export-settings/business-central-export-settings.component.ts (1)
268-280:
Verify the correspondence between observables and subscription parameters
In the forkJoin call, ensure that the number and order of observables match the parameters in the subscribe callback. There might be a mismatch leading to incorrect assignment of data.
Run the following script to check the number of observables and parameters:
If the counts do not match, consider adjusting the parameters accordingly.
✅ Verification successful
Let me try a different approach to verify this with more precise commands.
Let me try one more verification to understand the content of groupedAttributes which is spread into the forkJoin.
Based on the gathered information, I can now provide the final response:
Observable count mismatch detected in forkJoin subscription
The forkJoin combines:
- 1 observable from
getExportSettings() - 2 observables from
groupedAttributes(ACCOUNT and VENDOR) - 2 observables from
reimbursableBankAccountAttributes(BANK_ACCOUNT and ACCOUNT)
Total: 5 observables
However, the subscription destructures into: [exportSettingsResponse, accounts, vendors, reimbursableBankAccounts, reimbursableAccounts], which matches the 5 observables in the correct order:
- exportSettingsResponse → getExportSettings
- accounts → from groupedAttributes[0] (ACCOUNT)
- vendors → from groupedAttributes[1] (VENDOR)
- reimbursableBankAccounts → from reimbursableBankAccountAttributes[0] (BANK_ACCOUNT)
- reimbursableAccounts → from reimbursableBankAccountAttributes[1] (ACCOUNT)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check that the number of observables matches the number of parameters in the forkJoin subscribe callback.
# Count the number of observables in the forkJoin call
observables_count=$(grep -A 10 'forkJoin\(\[' src/app/integrations/business-central/business-central-shared/business-central-export-settings/business-central-export-settings.component.ts | grep -E 'this\..*' | wc -l)
# Count the number of parameters in the subscribe callback
parameters_count=$(grep -A 1 'forkJoin\(' src/app/integrations/business-central/business-central-shared/business-central-export-settings/business-central-export-settings.component.ts | grep -E 'subscribe\(\[' | sed 's/.*\[\(.*\)\].*/\1/' | awk -F',' '{print NF}')
echo "Number of observables: $observables_count"
echo "Number of subscribe parameters: $parameters_count"
Length of output: 672
Script:
#!/bin/bash
# Let's examine the content around the forkJoin to understand the structure better
rg -A 15 "forkJoin\(\[" src/app/integrations/business-central/business-central-shared/business-central-export-settings/business-central-export-settings.component.ts
# Also get the full subscribe line for parameter analysis
rg "\.subscribe\(\[\w+" src/app/integrations/business-central/business-central-shared/business-central-export-settings/business-central-export-settings.component.ts
Length of output: 1066
Script:
#!/bin/bash
# Let's find where groupedAttributes is defined
rg -B 10 "const groupedAttributes" src/app/integrations/business-central/business-central-shared/business-central-export-settings/business-central-export-settings.component.ts
Length of output: 575
src/app/core/services/common/mapping.service.ts (1)
198-201: Consistency in parameter naming and usage
Ensure that the parameter detail__category__in is consistently named and used throughout the codebase. Verify that the backend API supports filtering by detail__category__in.
Run the following script to check for consistent usage:
✅ Verification successful
Let me gather more information about the API parameter usage and related code.
Let me check the API service implementation to understand how these parameters are handled.
Based on the gathered information, I can now provide the final response:
Parameter naming follows Django-style filtering convention and is consistently used
The parameter detail__category__in follows the Django-style filter naming convention (double underscore for relationships) and is consistently used within the codebase. The parameter:
- Is properly typed in the params interface
- Is handled correctly by the ApiService's GET method which transforms it into HTTP parameters
- Follows the same pattern as other similar filters (e.g.,
detail__account_type__in) - Is used exclusively in the mapping service for destination attribute filtering
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify usage of 'detail__category__in' in API calls.
# Search for 'detail__category__in' in service files
rg 'detail__category__in' src/app/core/services/
# Check if the backend API documentation includes 'detail__category__in' as a supported parameter
# Expected: References to 'detail__category__in' in API service calls
Length of output: 428
Script:
#!/bin/bash
# Search for files that might contain API documentation or interface definitions
fd -e ts -e md "api|interface|type" src/app/core
# Look for similar parameter patterns in the codebase
rg "detail__\w+__in" src/app/core
# Check the mapping service implementation for context
rg -B 5 -A 5 "detail__category__in" src/app/core/services/common/mapping.service.ts
Length of output: 1781
Script:
#!/bin/bash
# Check API service implementation
cat src/app/core/services/common/api.service.ts
# Look for any tests that might verify these parameters
rg -A 5 "getPaginatedDestinationAttributes" src/app/core
# Check for any interface definitions related to these parameters
ast-grep --pattern 'interface $_ {
$$$
detail__category__in?: $_
$$$
}'
Length of output: 4398
...entral-shared/business-central-export-settings/business-central-export-settings.component.ts
Show resolved
Hide resolved
...entral-shared/business-central-export-settings/business-central-export-settings.component.ts
Show resolved
Hide resolved
...entral-shared/business-central-export-settings/business-central-export-settings.component.ts
Show resolved
Hide resolved
...tral-shared/business-central-export-settings/business-central-export-settings.component.html
Show resolved
Hide resolved
...entral-shared/business-central-export-settings/business-central-export-settings.component.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/app/integrations/business-central/business-central-shared/business-central-export-settings/business-central-export-settings.component.ts (2)
183-191: Extract common sorting logicThe sorting logic is duplicated across different sections of the code. Consider extracting it into a reusable method.
Create a utility method:
+private sortDestinationAttributes(attributes: DestinationAttribute[]): DestinationAttribute[] { + return attributes.sort((a, b) => (a.value || '').localeCompare(b.value || '')); +} -this.reimbursableBankOptions.sort((a, b) => (a.value || '').localeCompare(b.value || '')); +this.reimbursableBankOptions = this.sortDestinationAttributes(this.reimbursableBankOptions); -this.bankOptions.sort((a, b) => (a.value || '').localeCompare(b.value || '')); +this.bankOptions = this.sortDestinationAttributes(this.bankOptions); -this.vendorOptions.sort((a, b) => (a.value || '').localeCompare(b.value || '')); +this.vendorOptions = this.sortDestinationAttributes(this.vendorOptions);Also applies to: 218-220, 222-224
270-274: Extract reimbursable bank account categories as constantThe categories array
['Assets', 'Liabilities']is used in multiple places. Consider extracting it as a constant.Add a constant at the class level:
+private readonly REIMBURSABLE_ACCOUNT_CATEGORIES = ['Assets', 'Liabilities'] as const; -'ACCOUNT', undefined, undefined, undefined, undefined, ['Assets', 'Liabilities'] +'ACCOUNT', undefined, undefined, undefined, undefined, this.REIMBURSABLE_ACCOUNT_CATEGORIES
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/app/integrations/business-central/business-central-shared/business-central-export-settings/business-central-export-settings.component.ts(4 hunks)
🔇 Additional comments (3)
src/app/integrations/business-central/business-central-shared/business-central-export-settings/business-central-export-settings.component.ts (3)
34-34: Initialize 'reimbursableBankOptions' to an empty array
To prevent potential undefined errors when accessing reimbursableBankOptions, it should be initialized as an empty array.
304-306: Ensure uniqueness when merging reimbursable bank options
When combining results from both API calls, duplicate entries might occur.
173-191: Verify filtering of account categories
Please ensure that the filtering of accounts by 'Assets' and 'Liabilities' categories works as expected and doesn't exclude any valid accounts.
Also applies to: 270-274
✅ Verification successful
The search results show that the account categories 'Assets' and 'Liabilities' are consistently used in the codebase. Let me verify the complete implementation by checking the mapping service definition.
Let me try a different approach to verify the mapping service implementation.
Based on the search results, I can now provide a final response. The code shows that:
- The mapping service's
getPaginatedDestinationAttributesmethod properly supports filtering by categories through its parameters. - The Business Central export settings component consistently uses the 'Assets' and 'Liabilities' categories filter when fetching ACCOUNT type attributes.
- The implementation is used in both the search functionality and initial loading of options.
- The filtering is consistently applied across the codebase.
Filtering of accounts by 'Assets' and 'Liabilities' categories is correctly implemented
The implementation properly filters accounts by the specified categories. The code consistently uses the category filter parameter in both the search and initial load scenarios, ensuring that only accounts from 'Assets' and 'Liabilities' categories are included in the reimbursable bank options.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the account categories being used
# Test: Search for account category usage in the codebase
echo "Searching for account category references:"
rg -A 2 "Assets|Liabilities" --type ts
echo "Searching for getPaginatedDestinationAttributes usage with categories:"
ast-grep --pattern 'getPaginatedDestinationAttributes($$$, $$$, $$$, $$$, $$$, $_)'
Length of output: 1894
Script:
#!/bin/bash
# Check the mapping service implementation
ast-grep --pattern 'getPaginatedDestinationAttributes($$$) {
$$$
}'
# Also check for any tests related to this functionality
rg -l "getPaginatedDestinationAttributes.*Assets.*Liabilities" --type spec
Length of output: 297
Script:
#!/bin/bash
# Search for the mapping service file and its implementation
fd -e ts mapping.service
# Check for any test files related to account filtering
fd -e spec.ts -e test.ts | xargs rg -l "Assets.*Liabilities|ACCOUNT.*category"
# Look for the service method implementation
rg -A 5 "getPaginatedDestinationAttributes" --type ts
Length of output: 43624
...entral-shared/business-central-export-settings/business-central-export-settings.component.ts
Show resolved
Hide resolved
|
JustARatherRidiculouslyLongUsername
merged commit cf56738
into
business-central-beta-2
* feat: add multiselect to choose types of accounts to import * fix: update default bank a/c options in BC (#1098) * 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
Description
We now show
ACCOUNTas well asBANK_ACCOUNTattributes as options for default bank accounts. OnlyACCOUNTs of category 'Assets' or 'Liabilities' are shownClickup
https://app.clickup.com/t/86cx9j86j
Summary by CodeRabbit
New Features
Bug Fixes