fix: added special conditional checks for intacct and qbo for supporting card num and employee name #1119
Conversation
WalkthroughThe changes in this pull request primarily involve modifications 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 (
|
![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 (2)
src/app/core/models/common/advanced-settings.model.ts (1)
91-94: Improve app name comparison robustnessThe current implementation has several potential issues:
- Case-sensitive string comparison could be fragile
- String literals are used instead of enum values
- Complex conditional logic could be simplified
Consider this improved implementation:
- if (cccExportType && ['netsuite', 'quickbooks online', 'sage intacct'].includes(appName.toLowerCase())) { - return defaultOptions; // Allow all options including 'card_number' - } - return defaultOptions.filter(option => option !== 'card_number'); // Omit 'card_number' for other apps + const supportedApps = [ + AppName.NETSUITE.toLowerCase(), + AppName.QBO.toLowerCase(), + AppName.INTACCT.toLowerCase() + ]; + return cccExportType && supportedApps.includes(appName.toLowerCase()) + ? defaultOptions + : defaultOptions.filter(option => option !== 'card_number');src/app/integrations/intacct/intacct-shared/intacct-advanced-settings/intacct-advanced-settings.component.ts (1)
Line range hint
192-208: Consider enhancing memo preview with real dataThe preview functionality uses hardcoded values (e.g., "John Doe", "**** 3456"). Consider enhancing it to use real sample data from the configuration when available, especially for card numbers and employee names.
const previewValues: { [key: string]: string } = { - employee_email: '[email protected]', - employee_name: 'John Doe', - card_number: '**** 3456', + employee_email: this.configuration?.sample_employee_email || '[email protected]', + employee_name: this.configuration?.sample_employee_name || 'John Doe', + card_number: this.configuration?.sample_card_number || '**** 3456', category: 'Meals and Entertainment', purpose: 'Client Meeting', merchant: 'Pizza Hut',
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/app/core/models/common/advanced-settings.model.ts(2 hunks)src/app/integrations/intacct/intacct-shared/intacct-advanced-settings/intacct-advanced-settings.component.ts(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/app/core/models/common/advanced-settings.model.ts
[error] 80-80: 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 (4)
src/app/core/models/common/advanced-settings.model.ts (2)
7-7: LGTM: Import addition is appropriate
The IntacctConfiguration import is correctly added and necessary for the type changes in getMemoOptions.
83-85: Verify type casting and error handling
The type casting to IntacctConfiguration should be verified to ensure it's safe in all scenarios.
✅ Verification successful
Let me gather more information about the type definitions and usage to verify the type casting safety.
Based on the gathered information, I can now provide the final response:
Type casting in getMemoOptions is safe and properly handled
The type casting to IntacctConfiguration is safe because:
- The method uses type discrimination with
appName === AppName.INTACCTbefore the cast - The type definitions show that IntacctConfiguration and ExportSettingGet have different structures
- The code uses proper type guards with 'configurations' in exportSettings to handle different types
- Error handling is implicit through the optional chaining (?.) and nullish coalescing (??) operators
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all usages of getMemoOptions to verify correct parameter types
rg -A 2 "getMemoOptions\("
Length of output: 3038
Script:
#!/bin/bash
# Search for IntacctConfiguration type definition and related types
ast-grep --pattern 'type IntacctConfiguration = $_'
# Search for the complete class/interface definition
ast-grep --pattern 'interface IntacctConfiguration {
$$$
}'
# Search for ExportSettingGet type definition
ast-grep --pattern 'type ExportSettingGet = $_'
# Look for configurations property usage
rg "'configurations' in" -A 2
Length of output: 3115
src/app/integrations/intacct/intacct-shared/intacct-advanced-settings/intacct-advanced-settings.component.ts (2)
96-96: LGTM: Default initialization looks good
The initialization of defaultMemoFields using AdvancedSettingsModel.getDefaultMemoOptions() provides a good default value before the configuration is loaded.
316-316: Verify configuration object fields for memo options
The update to use getMemoOptions with configuration and AppName.INTACCT looks good and aligns with the PR objectives for supporting card number and employee name in memo fields.
Let's verify that the configuration object contains all required fields for memo options:
✅ Verification successful
Based on the search results, I can now generate the final response since I have enough information to verify the configuration object and memo options implementation:
Configuration object contains required fields for memo options
The implementation looks correct and safe:
- The Configuration interface in
advanced-settings.model.tsincludes the requiredmemo_structurefield - The
getMemoOptionsmethod is properly used across different components (Intacct, QBO, NetSuite) - The memo structure is correctly:
- Stored in configuration object
- Used in form initialization with
setDescriptionFieldcontrol - Validated with required validator
- Watched for changes to update preview
- Default memo fields include all necessary options including card number and employee name
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if configuration object contains required fields for memo options
# Search for configuration type definition and usage
ast-grep --pattern 'interface Configuration {
$$$
employee_field_mapping: $_
$$$
}'
# Search for getMemoOptions implementation to verify field usage
ast-grep --pattern 'getMemoOptions(configuration: $_, appName: $_) {
$$$
}'
Length of output: 171
Script:
#!/bin/bash
# Let's try a different approach to find the configuration interface and getMemoOptions implementation
# Search for Configuration interface
rg -A 10 "interface Configuration"
# Search for getMemoOptions implementation
rg -A 10 "getMemoOptions.*configuration.*appName"
# Search for defaultMemoFields usage
rg "defaultMemoFields.*getMemoOptions"
Length of output: 1590
Script:
#!/bin/bash
# Let's find the implementation of getMemoOptions in AdvancedSettingsModel
# Search for getMemoOptions static method in advanced-settings.model.ts
rg -A 15 "static getMemoOptions" src/app/core/models/intacct/intacct-configuration/advanced-settings.model.ts
# Also check how memo_structure is used
rg "memo_structure" src/app/core/models/intacct/intacct-configuration/advanced-settings.model.ts
Length of output: 301
Script:
#!/bin/bash
# Let's try a broader search to find the getMemoOptions implementation and its usage
# Search for getMemoOptions in the entire codebase with more context
rg -B 5 -A 15 "getMemoOptions"
# Search for setDescriptionField to understand memo structure setup
rg -B 5 -A 10 "setDescriptionField"
Length of output: 40735
|
* 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]>
* 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]>
* 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]>
Description
Clickup
Summary by CodeRabbit
New Features
Bug Fixes
Refactor