-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: initialize chart of accounts multiselect when there is no api response #1110
fix: initialize chart of accounts multiselect when there is no api response #1110
Conversation
WalkthroughThe changes involve a modification to the 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 (
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/app/core/models/business-central/business-central-configuration/business-central-import-settings.model.ts (2)
29-29
: Consider simplifying the nested ternary expression.The current implementation could be made more readable by using nullish coalescing and extracting the default value.
- chartOfAccountTypes: new FormControl(importSettings?.import_settings?.charts_of_accounts ? importSettings?.import_settings?.charts_of_accounts : ['Expense']), + const DEFAULT_CHART_OF_ACCOUNT_TYPE = ['Expense']; + chartOfAccountTypes: new FormControl(importSettings?.import_settings?.charts_of_accounts ?? DEFAULT_CHART_OF_ACCOUNT_TYPE),
29-29
: Consider centralizing default values.The default chart of accounts type ('Expense') appears in both the form initialization and
getChartOfAccountTypesList()
. Consider creating a constants file to maintain these values in one place.This would improve maintainability and ensure consistency across the application. Example structure:
// business-central.constants.ts export const BUSINESS_CENTRAL_DEFAULTS = { CHART_OF_ACCOUNTS: { DEFAULT_TYPE: 'Expense', TYPES: ['Expense', 'Assets', 'Income', 'Equity', 'Liabilities', 'Others', 'Cost of Goods Sold'] } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/app/core/models/business-central/business-central-configuration/business-central-import-settings.model.ts
(1 hunks)
🔇 Additional comments (2)
src/app/core/models/business-central/business-central-configuration/business-central-import-settings.model.ts (2)
29-29
: LGTM! Null safety improvement implemented correctly.
The additional null check for import_settings
prevents potential runtime errors when the API response is empty or malformed.
29-29
: Verify the impact on dependent components.
Let's ensure this initialization change doesn't affect components that consume this form control.
✅ Verification successful
The initialization change is safe and consistent with the codebase
The verification shows that:
- The form control
chartOfAccountTypes
is properly bound to a multi-select component in the template - The default value
['Expense']
aligns with the UI's behavior where "Expense" is disabled and pre-selected - The model correctly maps between the form control and the API property
charts_of_accounts
- The initialization handles both undefined and defined cases appropriately
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for components using chartOfAccountTypes
echo "Searching for components using chartOfAccountTypes..."
rg -l "chartOfAccountTypes" --type ts
# Search for template files that might bind to this control
echo "Searching for template bindings..."
rg -l "chartOfAccountTypes" --type html
# Look for any tests that might need updating
echo "Searching for related test files..."
rg -l "chartOfAccountTypes.*FormControl" --type spec
Length of output: 2526
Script:
#!/bin/bash
# Let's check the actual usage of chartOfAccountTypes in the components
echo "Checking Business Central import settings component implementation..."
rg -A 5 "chartOfAccountTypes" src/app/integrations/business-central/business-central-shared/business-central-import-settings/business-central-import-settings.component.ts
echo "Checking Business Central import settings template..."
cat src/app/integrations/business-central/business-central-shared/business-central-import-settings/business-central-import-settings.component.html
echo "Checking Business Central import settings model usage..."
rg -A 5 "charts_of_accounts" src/app/core/models/business-central/business-central-configuration/business-central-import-settings.model.ts
Length of output: 7752
* fix: initialize chart of accounts multiselect when there is no api response (#1110) * fix: remove the posted at date option for ccc expenses grouped by report (#1105) * fix: update login error flow and fix redirect url (#1117) * fix: restrict JE modules to group by expense only (#1113) * fix: restrict JE modules to group by expense only * fix: add a default bank account field for CCC expenses (#1114) * fix: remove validation temporarily (#1111) * fix: add a default bank account field for CCC expenses * fix: add missing options to bank accounts on page init * fix: dynamic content for xero customize settings (#1112) * fix: update sublabel key to avoid build fail (#1116) * fix: Prod fixes of QBD direct (#1118) * fix bugs (#1119) * refactor: capitalization * fix: only ccc exports not being saved (#1121) --------- Co-authored-by: Ashwin Thanaraj <[email protected]> Co-authored-by: Nilesh Pant <[email protected]> Co-authored-by: Dhaarani <[email protected]> Co-authored-by: Anish Kr Singh <[email protected]> --------- Co-authored-by: Ashwin Thanaraj <[email protected]> Co-authored-by: Nilesh Pant <[email protected]> Co-authored-by: Dhaarani <[email protected]> Co-authored-by: Anish Kr Singh <[email protected]> --------- Co-authored-by: Ashwin Thanaraj <[email protected]> Co-authored-by: Nilesh Pant <[email protected]> Co-authored-by: Dhaarani <[email protected]> Co-authored-by: Anish Kr Singh <[email protected]>
app.clickup.com
Summary by CodeRabbit