feat: add card num and employee name #1086
Conversation
WalkthroughThe changes in this pull request involve updates to multiple components and models related to advanced settings across different integrations. Key modifications include the addition of new memo options, specifically the string 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 (
|
![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 (5)
src/app/integrations/qbd/qbd-shared/qbd-advanced-setting/qbd-advanced-setting.component.ts (1)
Line range hint
103-112: Review memo preview formatting logicThe current implementation might have issues with new fields because:
- It relies on
defaultMemoFieldsindex for ordering- New fields might not be properly ordered if not included in
defaultMemoFieldsConsider refactoring to use a more robust approach:
- this.memoStructure.forEach((field, index) => { - if (field in previewValues) { - const defaultIndex = this.defaultMemoFields.indexOf(this.memoStructure[index]); - memo[defaultIndex] = previewValues[field]; - } - }); + this.memoStructure.forEach((field) => { + if (field in previewValues) { + memo.push(previewValues[field]); + } + });src/app/integrations/sage300/sage300-shared/sage300-advanced-settings/sage300-advanced-settings.component.ts (1)
Line range hint
63-63: Add new fields to defaultMemoOptions arrayThe new fields
employee_nameandcard_numberneed to be added to the defaultMemoOptions array to be properly supported in the memo preview generation.- defaultMemoOptions: string[] = ['employee_email', 'purpose', 'category', 'spent_on', 'report_number', 'expense_link']; + defaultMemoOptions: string[] = ['employee_email', 'employee_name', 'card_number', 'purpose', 'category', 'spent_on', 'report_number', 'expense_link'];src/app/core/models/netsuite/netsuite-configuration/netsuite-advanced-settings.model.ts (2)
Line range hint
52-57: Fix typo in property name:schedulEnabled→scheduleEnabledThe property name
schedulEnabledappears to be missing an 'e'. This should bescheduleEnabledfor consistency with other schedule-related properties in the codebase.Apply this diff to fix the property name:
export type NetsuiteAdvancedSettingAddEmailModel = { workspaceId: number; hours: number; - schedulEnabled: boolean; + scheduleEnabled: boolean; selectedEmails: string[]; }
Property naming inconsistency found across different integration models
The verification reveals inconsistent property naming across different integration models:
schedulEnabledis used in QBO, Xero, and Netsuite modelsscheduleEnabled(with 'e') is used in Sage300 and Business Central modelsThis inconsistency should be standardized across all integration models for better maintainability.
Locations requiring attention:
src/app/core/models/qbo/qbo-configuration/qbo-advanced-setting.model.tssrc/app/core/models/xero/xero-configuration/xero-advanced-settings.model.tssrc/app/core/models/netsuite/netsuite-configuration/netsuite-advanced-settings.model.ts🔗 Analysis chain
Line range hint
52-57: Verify all occurrences of the property nameLet's ensure all usages of this property are updated consistently across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all occurrences of schedulEnabled/scheduleEnabled to ensure consistent naming echo "Searching for current property name 'schedulEnabled':" rg "schedulEnabled" --type ts echo -e "\nSearching for correct property name 'scheduleEnabled':" rg "scheduleEnabled" --type tsLength of output: 1981
src/app/integrations/intacct/intacct-shared/intacct-advanced-settings/intacct-advanced-settings.component.ts (1)
Line range hint
89-89: Integration issue: New memo fields not included in defaultMemoFieldsThe new fields
employee_nameandcard_numberare added topreviewValuesbut are not included indefaultMemoFields. This will prevent them from appearing in the memo preview as theformatMemoPreviewmethod relies ondefaultMemoFieldsfor field ordering.Apply this diff to add the new fields:
- defaultMemoFields: string[] = ['employee_email', 'merchant', 'purpose', 'category', 'spent_on', 'report_number', 'expense_link']; + defaultMemoFields: string[] = ['employee_email', 'employee_name', 'card_number', 'merchant', 'purpose', 'category', 'spent_on', 'report_number', 'expense_link'];Also applies to: 166-167
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
src/app/core/models/common/advanced-settings.model.ts(1 hunks)src/app/core/models/netsuite/netsuite-configuration/netsuite-advanced-settings.model.ts(1 hunks)src/app/integrations/intacct/intacct-shared/intacct-advanced-settings/intacct-advanced-settings.component.ts(1 hunks)src/app/integrations/qbd/qbd-shared/qbd-advanced-setting/qbd-advanced-setting.component.ts(1 hunks)src/app/integrations/sage300/sage300-shared/sage300-advanced-settings/sage300-advanced-settings.component.ts(1 hunks)
🔇 Additional comments (4)
src/app/core/models/common/advanced-settings.model.ts (1)
72-72: LGTM! Verify the integration of the new card_number field.
The addition of 'card_number' to default memo options aligns with the PR objectives. The corresponding preview value is already properly defined.
Let's verify that the new field is properly handled throughout the codebase:
✅ Verification successful
Card number field is properly integrated across components
The verification shows that the card_number field is consistently implemented:
- All integration components (QBD, Sage300, Intacct) have the proper preview value setup
- The field is included in the default memo fields for QBD Direct advanced settings
- It's properly added to Netsuite advanced settings default memo options
- The implementation is consistent with the existing pattern across all components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the integration of card_number field in memo-related components and templates
# Search for card_number usage in component files
echo "Checking card_number usage in components:"
rg -l "card_number" --type ts --type html
# Search for any TODOs or FIXMEs related to card number implementation
echo "Checking for any pending TODOs related to card number:"
rg -i "todo.*card.*number|fixme.*card.*number"
Length of output: 847
Script:
#!/bin/bash
# Let's verify the implementation of card_number in the components that use it
# Check the implementation in QBD component
echo "=== QBD Advanced Settings Component ==="
rg -A 5 -B 5 "card_number" src/app/integrations/qbd/qbd-shared/qbd-advanced-setting/qbd-advanced-setting.component.ts
# Check the implementation in Sage300 component
echo -e "\n=== Sage300 Advanced Settings Component ==="
rg -A 5 -B 5 "card_number" src/app/integrations/sage300/sage300-shared/sage300-advanced-settings/sage300-advanced-settings.component.ts
# Check the implementation in Intacct component
echo -e "\n=== Intacct Advanced Settings Component ==="
rg -A 5 -B 5 "card_number" src/app/integrations/intacct/intacct-shared/intacct-advanced-settings/intacct-advanced-settings.component.ts
# Check if card_number is properly handled in the models
echo -e "\n=== Advanced Settings Models ==="
rg -A 5 -B 5 "card_number" src/app/core/models/netsuite/netsuite-configuration/netsuite-advanced-settings.model.ts src/app/core/models/qbd-direct/qbd-direct-configuration/qbd-direct-advanced-settings.model.ts
Length of output: 4886
src/app/integrations/qbd/qbd-shared/qbd-advanced-setting/qbd-advanced-setting.component.ts (1)
Line range hint 35-37: Verify integration of new memo fields
The new fields (card_number and employee_name) are added to preview values but card_number is not included in defaultMemoFields. This might cause inconsistencies in memo preview generation.
Run the following script to verify the usage of these fields across the codebase:
src/app/core/models/netsuite/netsuite-configuration/netsuite-advanced-settings.model.ts (1)
70-70: LGTM: Card number added to memo options
The addition of 'card_number' to the default memo options aligns with the PR objectives and maintains a logical structure.
src/app/integrations/intacct/intacct-shared/intacct-advanced-settings/intacct-advanced-settings.component.ts (1)
166-167: Verify the integration of new memo fields
Let's ensure these new fields are properly integrated across the codebase.
✅ Verification successful
Card number and employee name fields are properly integrated
The verification shows that these fields are part of the standard memo field structure across multiple components in the codebase:
- Both fields are included in default memo options in multiple components:
- Common advanced settings model
- NetSuite configuration
- QBD Direct configuration
- Sage300 advanced settings
- Intacct advanced settings
The card number format '1234 5678 9012 3456' is consistently used as a preview/sample value across these implementations, and employee_name is properly included in the default memo fields array where applicable.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage and implementation of new memo fields
# Check for other occurrences of memo field definitions
echo "Checking for other memo field definitions:"
rg -l "defaultMemoFields.*=.*\[.*\]"
# Check for any existing card number masking implementations
echo "Checking for existing card number masking patterns:"
rg -l "card.*number.*mask"
rg -l "\*{4}"
# Check for employee name field usage
echo "Checking for employee name field usage:"
rg -l "employee_name.*memo"
Length of output: 748
Script:
#!/bin/bash
# Let's examine the memo field implementations in both files to understand the context better
echo "Checking memo field implementations in Intacct:"
rg -A 10 "defaultMemoFields" src/app/integrations/intacct/intacct-shared/intacct-advanced-settings/intacct-advanced-settings.component.ts
echo -e "\nChecking memo field implementations in QBD:"
rg -A 10 "defaultMemoFields" src/app/integrations/qbd/qbd-shared/qbd-advanced-setting/qbd-advanced-setting.component.ts
# Let's also check for any card number validation or formatting patterns
echo -e "\nChecking for card number validation patterns:"
rg "card.*number|creditcard|payment.*card" -A 3
Length of output: 13050
| employee_name: 'John Doe', | ||
| card_number: '1234 5678 9012 3456', |
There was a problem hiding this comment.
Consider masking sensitive card number information in preview
While adding card number to memo preview is useful, displaying the full card number might pose security risks. Consider:
- Masking the card number to show only the last 4 digits
- Adding a tooltip or note indicating this is a masked preview
Apply this diff to mask the card number:
- card_number: '1234 5678 9012 3456',
+ card_number: '•••• •••• •••• 3456',📝 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.
| employee_name: 'John Doe', | |
| card_number: '1234 5678 9012 3456', | |
| employee_name: 'John Doe', | |
| card_number: '•••• •••• •••• 3456', |
| employee_name: 'John Doe', | ||
| card_number: '1234 5678 9012 3456', |
There was a problem hiding this comment.
Security concern: Credit card number should be masked
The card number is currently displayed in full (1234 5678 9012 3456) in the memo preview. This could expose sensitive financial information and potentially violate PCI DSS requirements.
Apply this diff to mask the card number:
- card_number: '1234 5678 9012 3456',
+ card_number: '•••• •••• •••• 3456',📝 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.
| employee_name: 'John Doe', | |
| card_number: '1234 5678 9012 3456', | |
| employee_name: 'John Doe', | |
| card_number: '•••• •••• •••• 3456', |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
src/app/core/models/sage300/sage300-configuration/sage300-advanced-settings.model.ts (1)
24-24: Fix comma spacing in defaultMemoOptions arrayThere's a missing space after the comma between 'employee_email' and 'employee_name'.
- const defaultMemoOptions: string[] = ['employee_email', 'employee_name','purpose', 'category', 'spent_on', 'report_number', 'expense_link', 'card_number']; + const defaultMemoOptions: string[] = ['employee_email', 'employee_name', 'purpose', 'category', 'spent_on', 'report_number', 'expense_link', 'card_number'];🧰 Tools
🪛 eslint
[error] 24-24: A space is required after ','.
(comma-spacing)
🪛 GitHub Check: lint
[failure] 24-24:
A space is required after ','src/app/core/models/business-central/business-central-configuration/business-central-advanced-settings.model.ts (1)
24-24: Fix comma spacing in defaultMemoOptions arrayAdd a space after the comma between 'employee_name' and 'purpose'.
-const defaultMemoOptions: string[] = ['employee_email', 'employee_name','purpose', 'category', 'spent_on', 'report_number', 'expense_link', 'card_number']; +const defaultMemoOptions: string[] = ['employee_email', 'employee_name', 'purpose', 'category', 'spent_on', 'report_number', 'expense_link', 'card_number'];🧰 Tools
🪛 eslint
[error] 24-24: A space is required after ','.
(comma-spacing)
🪛 GitHub Check: lint
[failure] 24-24:
A space is required after ','src/app/integrations/business-central/business-central-shared/business-central-advanced-settings/business-central-advanced-settings.component.ts (1)
50-50: Fix formatting: Add space after comma.Add a space after the comma between 'employee_name' and 'purpose' to maintain consistent formatting.
- defaultMemoOptions: string[] = ['employee_email', 'employee_name','purpose', 'category', 'spent_on', 'report_number', 'card_number']; + defaultMemoOptions: string[] = ['employee_email', 'employee_name', 'purpose', 'category', 'spent_on', 'report_number', 'card_number'];🧰 Tools
🪛 eslint
[error] 50-50: A space is required after ','.
(comma-spacing)
🪛 GitHub Check: lint
[failure] 50-50:
A space is required after ','src/app/integrations/sage300/sage300-shared/sage300-advanced-settings/sage300-advanced-settings.component.ts (1)
55-55: Fix comma spacing in defaultMemoOptions arrayThe addition of new memo fields is good, but there's a minor style issue to fix.
- defaultMemoOptions: string[] = ['employee_email', 'employee_name','purpose', 'category', 'spent_on', 'report_number', 'expense_link', 'card_number']; + defaultMemoOptions: string[] = ['employee_email', 'employee_name', 'purpose', 'category', 'spent_on', 'report_number', 'expense_link', 'card_number'];🧰 Tools
🪛 eslint
[error] 55-55: A space is required after ','.
(comma-spacing)
🪛 GitHub Check: lint
[failure] 55-55:
A space is required after ','
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
src/app/core/models/business-central/business-central-configuration/business-central-advanced-settings.model.ts(1 hunks)src/app/core/models/sage300/sage300-configuration/sage300-advanced-settings.model.ts(1 hunks)src/app/integrations/business-central/business-central-shared/business-central-advanced-settings/business-central-advanced-settings.component.ts(1 hunks)src/app/integrations/sage300/sage300-shared/sage300-advanced-settings/sage300-advanced-settings.component.ts(2 hunks)
🧰 Additional context used
🪛 eslint
src/app/core/models/business-central/business-central-configuration/business-central-advanced-settings.model.ts
[error] 24-24: A space is required after ','.
(comma-spacing)
src/app/core/models/sage300/sage300-configuration/sage300-advanced-settings.model.ts
[error] 24-24: A space is required after ','.
(comma-spacing)
src/app/integrations/business-central/business-central-shared/business-central-advanced-settings/business-central-advanced-settings.component.ts
[error] 50-50: A space is required after ','.
(comma-spacing)
src/app/integrations/sage300/sage300-shared/sage300-advanced-settings/sage300-advanced-settings.component.ts
[error] 55-55: A space is required after ','.
(comma-spacing)
🪛 GitHub Check: lint
src/app/core/models/business-central/business-central-configuration/business-central-advanced-settings.model.ts
[failure] 24-24:
A space is required after ','
src/app/core/models/sage300/sage300-configuration/sage300-advanced-settings.model.ts
[failure] 24-24:
A space is required after ','
src/app/integrations/business-central/business-central-shared/business-central-advanced-settings/business-central-advanced-settings.component.ts
[failure] 50-50:
A space is required after ','
src/app/integrations/sage300/sage300-shared/sage300-advanced-settings/sage300-advanced-settings.component.ts
[failure] 55-55:
A space is required after ','
🔇 Additional comments (2)
src/app/core/models/business-central/business-central-configuration/business-central-advanced-settings.model.ts (2)
Line range hint 31-38: LGTM! Payload creation handles new memo fields correctly
The createAdvancedSettingPayload method correctly handles the new memo fields as part of the memoStructure array without requiring any changes.
🧰 Tools
🪛 Biome (1.9.4)
[error] 21-42: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 27-27: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
🪛 eslint
[error] 24-24: A space is required after ','.
(comma-spacing)
🪛 GitHub Check: lint
[failure] 24-24:
A space is required after ','
Line range hint 24-28: Verify handling of new memo fields
The addition of 'employee_name' and 'card_number' to defaultMemoOptions looks good. Let's verify that these new fields are properly handled in the components that consume them.
✅ Verification successful
Based on the search results, I can see that both employee_name and card_number fields are properly handled across the codebase:
-
They are consistently included in the default memo options across different integration components:
- Business Central (
business-central-advanced-settings.model.ts) - Sage300 (
sage300-advanced-settings.component.ts) - QBD Direct (
qbd-direct-advanced-settings.model.ts) - NetSuite (
netsuite-advanced-settings.model.ts) - Intacct (
intacct-advanced-settings.component.ts)
- Business Central (
-
The fields are properly used in preview data structures with consistent mock values across components:
employee_name: 'John Doe', card_number: '1234 5678 9012 3456'
-
The
employee_namefield is properly handled in display components:- Dashboard accounting error dialog
- Dashboard Intacct errors component
-
Both fields are included in the common advanced settings model (
common/advanced-settings.model.ts), ensuring consistent behavior across integrations.
The new memo fields are properly integrated across the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that components properly handle the new memo fields
# Check for memo field handling in components
rg -A 5 "employee_name|card_number" --type ts
# Check for any potential template bindings
rg -A 5 "employee_name|card_number" --type html
# Check for any related test coverage
rg -A 5 "employee_name|card_number" --type spec
Length of output: 34423
🧰 Tools
🪛 Biome (1.9.4)
[error] 21-42: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 27-27: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
🪛 eslint
[error] 24-24: A space is required after ','.
(comma-spacing)
🪛 GitHub Check: lint
[failure] 24-24:
A space is required after ','
| @@ -47,7 +47,7 @@ | |||
|
|
|||
| ConfigurationCtaText = ConfigurationCta; | |||
|
|
|||
| defaultMemoOptions: string[] = ['employee_email', 'purpose', 'category', 'spent_on', 'report_number']; | |||
| defaultMemoOptions: string[] = ['employee_email', 'employee_name','purpose', 'category', 'spent_on', 'report_number', 'card_number']; | |||
There was a problem hiding this comment.
💡 Codebase verification
Inconsistency found in memo options between components
The verification revealed that the Business Central component is missing 'expense_link' in its defaultMemoOptions array, which is present in both the Sage300 component and the model definitions:
src/app/integrations/business-central/business-central-shared/business-central-advanced-settings/business-central-advanced-settings.component.ts: Missing 'expense_link'- Other components and models include: ['employee_email', 'employee_name', 'purpose', 'category', 'spent_on', 'report_number', 'expense_link', 'card_number']
Please align the defaultMemoOptions array with other components by adding the missing 'expense_link' option.
🔗 Analysis chain
LGTM! Addition of new memo fields.
The addition of 'employee_name' and 'card_number' to defaultMemoOptions aligns with the PR objectives. The component's existing memo preview and structure handling will automatically support these new fields.
Let's verify the integration of these new fields:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the new memo fields are properly integrated across the codebase
# Check if the memo fields are consistently defined across components
rg -l "defaultMemoOptions.*employee_name.*card_number" --type ts
# Check for any related template updates for these fields
rg -l "employee_name|card_number" --type html
Length of output: 785
Script:
#!/bin/bash
# Let's verify the consistency of memo field implementations
# Check the actual implementation in Sage300 component
rg "defaultMemoOptions.*=.*\[.*\]" src/app/integrations/sage300/sage300-shared/sage300-advanced-settings/sage300-advanced-settings.component.ts
# Check the model definitions
rg "defaultMemoOptions" src/app/core/models/sage300/sage300-configuration/sage300-advanced-settings.model.ts
rg "defaultMemoOptions" src/app/core/models/business-central/business-central-configuration/business-central-advanced-settings.model.ts
# Check the HTML templates for memo field usage
rg -A 5 "employee_name|card_number" src/app/shared/components/si/helper/dashboard-intacct-errors/dashboard-intacct-errors.component.html
rg -A 5 "employee_name|card_number" src/app/shared/components/dashboard/dashboard-accounting-error-dialog/dashboard-accounting-error-dialog.component.html
Length of output: 2061
🧰 Tools
🪛 eslint
[error] 50-50: A space is required after ','.
(comma-spacing)
🪛 GitHub Check: lint
[failure] 50-50:
A space is required after ','
| employee_name: 'John Doe', | ||
| card_number: '1234 5678 9012 3456', |
There was a problem hiding this comment.
Security: Mask sensitive card number information
The card number is currently displayed in full in the preview, which could expose sensitive information. This needs to be addressed before merging.
- card_number: '1234 5678 9012 3456',
+ card_number: '•••• •••• •••• 3456',Consider creating a utility function to consistently mask card numbers across the application:
private maskCardNumber(cardNumber: string): string {
if (!cardNumber) {
return '';
}
const lastFourDigits = cardNumber.slice(-4);
return `•••• •••• •••• ${lastFourDigits}`;
}💡 Codebase verification
Based on the analysis of the codebase, I can now generate the final response:
Card numbers should be masked in preview/sample data
The hardcoded card numbers appear in preview/sample data across multiple integration components. While these are just example values, it's better to use masked formats even in preview data to promote secure practices.
- Replace
1234 5678 9012 3456withXXXX XXXX XXXX 3456in:- src/app/integrations/sage300/sage300-shared/sage300-advanced-settings/sage300-advanced-settings.component.ts
- src/app/integrations/qbd/qbd-shared/qbd-advanced-setting/qbd-advanced-setting.component.ts
- src/app/integrations/intacct/intacct-shared/intacct-advanced-settings/intacct-advanced-settings.component.ts
- src/app/core/models/common/advanced-settings.model.ts
🔗 Analysis chain
Verify secure handling of card numbers throughout the application
Let's ensure card numbers are properly masked everywhere they appear.
Let me gather more context about how these card numbers are being used.
Let me check one more thing to understand if these are just preview/sample values or actual card data.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential exposure of card numbers in code
# Search for card number patterns in typescript files
rg -t ts "card.*number.*[:\"'].*\d{4}.*\d{4}.*\d{4}.*\d{4}"
# Search for potential logging of card numbers
rg -t ts "console\.log.*card.*number|log.*card.*number"
# Search for card number related variables and properties
ast-grep --pattern 'cardNumber: string'
Length of output: 694
Script:
#!/bin/bash
# Check the context where these card numbers appear
rg -B 5 -A 5 "card_number.*1234"
# Look for any masking utilities or functions
rg -t ts "mask|hideCard|obscure"
# Check for any interfaces or types related to card data
ast-grep --pattern 'interface $_ {
$$$
card_number: $_
$$$
}'
Length of output: 5882
Script:
#!/bin/bash
# Check for any preview or sample related context
rg -t ts "preview.*values|sample.*data" -B 2 -A 2
# Look for any actual card data handling
rg -t ts "card.*data|payment.*card|credit.*card" -B 2 -A 2
Length of output: 107902
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (10)
src/app/core/models/common/advanced-settings.model.ts (2)
78-92: Improve method implementation and documentation
- Replace
thiswith class name in static method to improve clarity- Add JSDoc comments to document the method's purpose and parameters
- static getMemoOptions(exportSettings: ExportSettingGet | NetSuiteExportSettingGet | QBOExportSettingGet, appName: string): string[] { - const defaultOptions = this.getDefaultMemoOptions(); + /** + * Returns memo options based on export settings and application name + * @param exportSettings - Export settings from various integrations + * @param appName - Name of the application + * @returns Array of memo options, conditionally including card_number + */ + static getMemoOptions(exportSettings: ExportSettingGet | NetSuiteExportSettingGet | QBOExportSettingGet, appName: string): string[] { + const defaultOptions = AdvancedSettingsModel.getDefaultMemoOptions();🧰 Tools
🪛 Biome (1.9.4)
[error] 79-79: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
Line range hint
134-139: Simplify filterAdminEmails implementationThe current implementation uses
find()with a side effect, which is not ideal. Consider usingfilter()for a cleaner implementation.static filterAdminEmails = (emailToSearch: string[], adminEmails: EmailOption[]) => { - const adminEmailsList: EmailOption[] = []; - for (const email of emailToSearch) { - adminEmails.find(item => (item.email === email ? adminEmailsList.push(item) : null)); - } - return adminEmailsList; + return adminEmails.filter(item => emailToSearch.includes(item.email)); };src/app/core/models/netsuite/netsuite-configuration/netsuite-advanced-settings.model.ts (2)
75-75: Replacethiswith class name in static methodUsing
thisin static methods can be confusing. For better clarity and maintainability, use the class name directly.- const defaultOptions = this.getDefaultMemoOptions(); + const defaultOptions = NetsuiteAdvancedSettingModel.getDefaultMemoOptions();🧰 Tools
🪛 Biome (1.9.4)
[error] 75-75: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
Line range hint
46-50: Maintain consistent naming conventionThe property
schedulEnabledinNetsuiteAdvancedSettingAddEmailModelshould follow the snake_case convention used throughout the codebase.export type NetsuiteAdvancedSettingAddEmailModel = { workspaceId: number; hours: number; - schedulEnabled: boolean; + schedule_enabled: boolean; selectedEmails: string[]; }src/app/integrations/qbo/qbo-shared/qbo-advanced-settings/qbo-advanced-settings.component.ts (2)
234-236: Consider improving array item indentationWhile the logic is correct, the indentation of the forkJoin array items could be improved for better readability.
forkJoin([ this.advancedSettingsService.getAdvancedSettings(), this.skipExportService.getExpenseFilter(), this.skipExportService.getExpenseFields('v1'), this.mappingService.getGroupedDestinationAttributes(['BANK_ACCOUNT'], 'v1', 'qbo'), this.configurationService.getAdditionalEmails(), - this.workspaceService.getWorkspaceGeneralSettings(), - this.exportSettingsService.getExportSettings() + this.workspaceService.getWorkspaceGeneralSettings(), + this.exportSettingsService.getExportSettings() ])
254-257: Consider adding tests for the new memo options functionalityWhile the implementation looks good, it would be beneficial to add tests covering:
- The integration with QboExportSettingsService
- The new memo options (card number and employee name)
- The behavior of getMemoOptions with different export settings
Would you like me to help generate test cases for these scenarios?
src/app/integrations/netsuite/netsuite-shared/netsuite-advanced-settings/netsuite-advanced-settings.component.ts (1)
Line range hint
301-304: Remove duplicate isLoading flag resetThe isLoading flag is set to false twice in succession:
- this.isLoading = false; this.setupFormWatchers(); - this.isLoading = false; + this.isLoading = false;src/app/integrations/qbo/qbo-onboarding/qbo-clone-settings/qbo-clone-settings.component.ts (3)
514-515: LGTM! Consider adding unit tests.The change to use
AdvancedSettingsModel.getMemoOptions()aligns with the PR objective of enhancing memo options. This allows dynamic memo options based on export settings, which is necessary for supporting card number and employee name in memos.Consider adding unit tests to verify:
- The memo options include card number and employee name when appropriate
- The memo preview updates correctly with the new options
- The memo structure updates correctly when options are selected/deselected
514-515: Consider enhancing memo-related functionality.While the memo functionality is well-structured, consider these improvements for better maintainability and reliability:
- Add type safety:
- memoStructure: string[] = []; - memoPreviewText: string = ''; + interface MemoOption { + key: string; + displayName: string; + } + memoStructure: MemoOption[] = []; + memoPreviewText: string = '';
- Add error handling in memo-related methods:
onMultiSelectChange() { const memo = this.advancedSettingForm.controls.memoStructure.value; + if (!memo) { + return; + } const changedMemo = AdvancedSettingsModel.formatMemoPreview(memo, this.defaultMemoOptions)[1]; this.advancedSettingForm.controls.memoStructure.patchValue(changedMemo); }
- Add JSDoc comments to document the memo structure format and usage:
/** * Handles changes to memo structure in the advanced settings form. * Updates the preview text when memo options are selected/deselected. * @private */ private createMemoStructureWatcher(): void { // ... existing code ... }Also applies to: 449-458, 459-466
512-517: Consider improving component initialization.The setup process in
setupPage()is complex and could benefit from better error handling and organization.
- Extract the memo initialization into a separate method:
private initializeMemoOptions(): void { this.defaultMemoOptions = AdvancedSettingsModel.getMemoOptions( this.cloneSetting.export_settings, AppName.QBO ); }
- Add error handling for the nested subscription:
private setupPage(): void { this.setupOnboardingSteps(); // ... existing code ... forkJoin([ this.cloneSettingService.getCloneSettings(), // ... other API calls ... ]).pipe( catchError(error => { this.isLoading = false; this.toastService.displayToastMessage( ToastSeverity.ERROR, 'Failed to load settings' ); return throwError(error); }) ).subscribe(/* ... */); }
- Consider using a loading state per form section for better UX:
interface LoadingState { employeeSettings: boolean; exportSettings: boolean; importSettings: boolean; advancedSettings: boolean; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
src/app/core/models/common/advanced-settings.model.ts(2 hunks)src/app/core/models/netsuite/netsuite-configuration/netsuite-advanced-settings.model.ts(2 hunks)src/app/integrations/netsuite/netsuite-shared/netsuite-advanced-settings/netsuite-advanced-settings.component.ts(5 hunks)src/app/integrations/qbo/qbo-onboarding/qbo-clone-settings/qbo-clone-settings.component.ts(1 hunks)src/app/integrations/qbo/qbo-shared/qbo-advanced-settings/qbo-advanced-settings.component.ts(4 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/app/core/models/common/advanced-settings.model.ts
[error] 79-79: 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/models/netsuite/netsuite-configuration/netsuite-advanced-settings.model.ts
[error] 75-75: 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 (10)
src/app/core/models/common/advanced-settings.model.ts (2)
4-6: LGTM: Import statements are correctly defined
The new imports for export settings models are necessary to support the type checking in the new getMemoOptions method.
75-76: LGTM: Default memo options updated as per requirements
The addition of 'card_number' to default memo options aligns with the PR objective to enhance memo options.
src/app/core/models/netsuite/netsuite-configuration/netsuite-advanced-settings.model.ts (2)
71-72: LGTM: Memo options enhanced as per requirements
The addition of 'card_number' and 'employee_name' to default memo options aligns with the PR objectives.
74-83: LGTM: Well-implemented memo options filtering
The method correctly implements conditional filtering of memo options based on export settings and app type, with proper type checking.
🧰 Tools
🪛 Biome (1.9.4)
[error] 75-75: 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/integrations/qbo/qbo-shared/qbo-advanced-settings/qbo-advanced-settings.component.ts (3)
22-22: LGTM!
The import statement follows Angular conventions and maintains consistency with the project structure.
97-98: LGTM!
The constructor injection follows Angular dependency injection best practices.
254-254: Verify the implementation of getMemoOptions
The code uses a new method to set memo options. Let's verify its implementation to ensure it properly includes the new card number and employee name fields.
✅ Verification successful
Implementation of getMemoOptions is properly handling card number field
The implementation in AdvancedSettingsModel correctly handles the memo options including the card number field:
- Default memo options include 'card_number' along with other fields like employee name, email, etc. (line 75)
- The
getMemoOptionsmethod:- Gets all default options including card number
- For QBO (and other supported apps like NetSuite and Sage Intacct), includes card number when corporate credit card expenses are configured
- For other cases, filters out the card number option
The implementation aligns with the requirements and properly handles the inclusion of card number based on the application type and configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of getMemoOptions method
# Look for the getMemoOptions method definition
ast-grep --pattern 'getMemoOptions($settings, $appName) {
$$$
}'
# Look for tests covering this method
rg -l "getMemoOptions.*test" -g "*.spec.ts"
Length of output: 114
Script:
#!/bin/bash
# Let's try a different approach to locate the getMemoOptions implementation
# Search for class definition containing getMemoOptions
ast-grep --pattern 'class AdvancedSettingsModel {
$$$
}'
# Also search for any occurrence of getMemoOptions
rg "getMemoOptions" -A 5
Length of output: 11112
src/app/integrations/netsuite/netsuite-shared/netsuite-advanced-settings/netsuite-advanced-settings.component.ts (3)
13-13: LGTM: Import and property declaration are well-structured
The new import and property declaration for export settings follow Angular best practices and are properly typed.
Also applies to: 56-56
117-118: LGTM: Constructor injection follows Angular DI patterns
The NetsuiteExportSettingsService is properly injected following Angular's dependency injection best practices.
275-275: Verify the new memo options are properly populated
The code adds support for new memo options through export settings, but let's verify that the card number and employee name options are properly included and displayed in the UI.
✅ Verification successful
Let me gather more specific information about the memo options implementation.
Let me check the actual implementation of the NetsuiteAdvancedSettingModel class to understand the memo options.
Memo options implementation is correctly handling card number and employee name
The implementation is working as expected:
- Default memo options include 'employee_name' and 'card_number' along with other fields
- The
getMemoOptionsmethod correctly filters options based on export settings and app name - Card number is only included for supported apps (NetSuite, QBO, Intacct) when corporate credit card expenses are configured
- The options are properly bound to the multi-select component in the template
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of new memo options
# Check for the presence of card_number and employee_name in memo options
rg -l "card_number|employee_name" --type ts
# Check for any related test files
fd -e spec.ts -e test.ts netsuite-advanced-settings
Length of output: 1951
Script:
#!/bin/bash
# Check the implementation of getMemoOptions in the NetsuiteAdvancedSettingModel
ast-grep --pattern 'getMemoOptions($$$) {
$$$
}'
# Check the test cases for memo options
rg -A 10 "getMemoOptions" src/app/integrations/netsuite/netsuite-shared/netsuite-advanced-settings/netsuite-advanced-settings.component.spec.ts
# Check the template for memo options rendering
rg -A 5 "defaultMemoOptions" src/app/integrations/netsuite/netsuite-shared/netsuite-advanced-settings/netsuite-advanced-settings.component.html
Length of output: 661
Script:
#!/bin/bash
# Check the NetsuiteAdvancedSettingModel implementation
rg -A 20 "class NetsuiteAdvancedSettingModel" src/app/core/models/netsuite/netsuite-configuration/netsuite-advanced-settings.model.ts
# Check specific memo options implementation
rg -B 2 -A 10 "static getMemoOptions" src/app/core/models/netsuite/netsuite-configuration/netsuite-advanced-settings.model.ts
Length of output: 1884
| if (cccExportType && ['netsuite', 'qbo', '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 |
There was a problem hiding this comment.
💡 Codebase verification
Consider using AppName enum instead of hardcoded strings
The codebase already has an AppName enum that includes all the required values:
AppName.NETSUITE('NetSuite')AppName.QBO('QuickBooks Online')AppName.INTACCT('Sage Intacct')
Replace the hardcoded string comparison with the enum to improve type safety and maintainability:
if (cccExportType && [AppName.NETSUITE, AppName.QBO, AppName.INTACCT].includes(appName)) {
return defaultOptions; // Allow all options including 'card_number'
}🔗 Analysis chain
Verify app name comparison logic
The app name comparison is case-insensitive but uses hardcoded strings. Consider using an enum or constants for app names to prevent typos and improve maintainability.
Let me analyze the findings from the codebase search to verify the app name comparison logic.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other occurrences of these app names to ensure consistency
rg -i "netsuite|qbo|sage intacct" --type ts
Length of output: 106758
Script:
#!/bin/bash
# Search for enum or constant definitions of app names
ast-grep --pattern 'enum AppName {
$$$
}'
Length of output: 876
.../netsuite/netsuite-shared/netsuite-advanced-settings/netsuite-advanced-settings.component.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
src/app/integrations/qbo/qbo-shared/qbo-advanced-settings/qbo-advanced-settings.component.ts (2)
69-69: Consider initializing defaultMemoOptions with common memo fieldsWhile initializing as an empty array works, consider providing default memo fields that are commonly used, to ensure the component has sensible defaults even before the export settings are loaded.
- defaultMemoOptions: string[] = []; + defaultMemoOptions: string[] = ['employee_email', 'purpose', 'category'];
254-255: Consider adding loading state for memo optionsThe memo options might take time to load. Consider adding a separate loading state for the memo preview section.
+ isMemoOptionsLoading: boolean = true; this.defaultMemoOptions = AdvancedSettingsModel.getMemoOptions(exportSettings, AppName.QBO); + this.isMemoOptionsLoading = false; this.setupFormWatchers(); this.isLoading = false;src/app/integrations/qbo/qbo-shared/qbo-advanced-settings/qbo-advanced-settings.component.spec.ts (1)
Line range hint
365-419: Add test coverage for new memo optionsThe memo-related test cases need to be enhanced to cover the new card number and employee name features. Consider adding:
- Test cases for card number in memo structure
- Test cases for employee name in memo structure
- Integration tests for both features combined
Add these test cases:
it('should include card number in memo structure', () => { const memoWithCard = [...mockMemo, { value: 'card_number', label: 'Card Number' }]; component.advancedSettingForm.patchValue({ memoStructure: memoWithCard }); component.onMultiSelectChange(); expect(component.memoPreviewText).toContain('**** 1234'); // Assuming masked card number }); it('should include employee name in memo structure', () => { const memoWithEmployee = [...mockMemo, { value: 'employee_name', label: 'Employee Name' }]; component.advancedSettingForm.patchValue({ memoStructure: memoWithEmployee }); component.onMultiSelectChange(); expect(component.memoPreviewText).toContain('John Doe'); // Assuming employee name });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/app/integrations/qbo/qbo-shared/qbo-advanced-settings/qbo-advanced-settings.component.spec.ts(6 hunks)src/app/integrations/qbo/qbo-shared/qbo-advanced-settings/qbo-advanced-settings.component.ts(5 hunks)
🔇 Additional comments (2)
src/app/integrations/qbo/qbo-shared/qbo-advanced-settings/qbo-advanced-settings.component.ts (1)
254-254: Verify card number integration in memo options
Since one of the PR objectives is to add card number to memo options, let's verify its integration.
✅ Verification successful
Card number integration in memo options is properly handled
The code shows that card number is correctly integrated in the memo options system:
- It's included in the default memo options list (
getDefaultMemoOptions) - The
getMemoOptionsmethod properly handles card number inclusion based on app type and CCC export type - The
formatMemoPreviewmethod includes support for card number preview with a sample value
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the integration of card_number in memo options
# Expected: Find references to card_number in memo-related code
# Check for card_number in memo options or related configuration
rg -l "card_number" --type ts
# Check for any existing usage of card number in memo preview
rg -l "formatMemoPreview.*card_number" --type ts
Length of output: 967
Script:
#!/bin/bash
# Let's check the implementation of getMemoOptions in AdvancedSettingsModel
ast-grep --pattern 'getMemoOptions($$$) {
$$$
}'
# Also check for card_number in QBO specific files
rg "card_number" --type ts src/app/core/models/qbo/ -A 3 -B 3
# Check QBO advanced settings model
rg "card_number" --type ts src/app/integrations/qbo/ -A 3 -B 3
Length of output: 177
Script:
#!/bin/bash
# Let's check the implementation in the common advanced settings model
cat src/app/core/models/common/advanced-settings.model.ts
# Check QBO specific advanced settings if exists
fd "qbo.*advanced-settings.model" --type f --exec cat {}
Length of output: 10836
src/app/integrations/qbo/qbo-shared/qbo-advanced-settings/qbo-advanced-settings.component.spec.ts (1)
Line range hint 135-148: Critical: Re-enable or replace disabled tests
Two important test cases have been disabled:
- Component and forms initialization
- Additional email options concatenation
These tests cover core functionality. If they're failing due to the new memo options:
- Update the tests to accommodate the new changes
- Add new test cases specifically for card number and employee name in memo options
Also applies to: 149-164
| this.workspaceService.getWorkspaceGeneralSettings(), | ||
| this.exportSettingsService.getExportSettings() | ||
| ]).subscribe(([sage300AdvancedSettingResponse, expenseFiltersGet, expenseFilterCondition, billPaymentAccounts, adminEmails, workspaceGeneralSettings, exportSettings]) => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for export settings failure
The forkJoin will fail entirely if any request fails. Consider using catchError operators on individual observables to handle failures gracefully.
forkJoin([
this.advancedSettingsService.getAdvancedSettings(),
this.skipExportService.getExpenseFilter(),
this.skipExportService.getExpenseFields('v1'),
this.mappingService.getGroupedDestinationAttributes(['BANK_ACCOUNT'], 'v1', 'qbo'),
this.configurationService.getAdditionalEmails(),
this.workspaceService.getWorkspaceGeneralSettings(),
- this.exportSettingsService.getExportSettings()
+ this.exportSettingsService.getExportSettings().pipe(
+ catchError(error => {
+ this.toastService.displayToastMessage(ToastSeverity.ERROR, 'Failed to load memo options');
+ return of([]); // Provide fallback empty settings
+ })
+ )
])Committable suggestion skipped: line range outside the PR's diff.
| @@ -19,11 +19,13 @@ import { AdvancedSettingsModel, ExpenseFilter, SkipExportModel } from 'src/app/c | |||
| import { GroupedDestinationAttribute } from 'src/app/core/models/db/destination-attribute.model'; | |||
| import { orgMockData } from 'src/app/core/services/org/org.fixture'; | |||
| import { OrgService } from 'src/app/core/services/org/org.service'; | |||
| import { QboExportSettingsService } from 'src/app/core/services/qbo/qbo-configuration/qbo-export-settings.service'; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage for QboExportSettingsService
The service is integrated but only includes getExportSettings in the spy object. Given that this PR enhances memo options, consider adding test coverage for:
- Handling of card number in memo options
- Employee name integration in memo structure
- Error scenarios for export settings retrieval
const exportSettingsServiceSpy = jasmine.createSpyObj('QboExportSettingsService', [
- 'getExportSettings'
+ 'getExportSettings',
+ 'getMemoOptions',
+ 'updateMemoStructure'
]);Also applies to: 28-28, 41-41, 61-61, 77-77
.../business-central/business-central-configuration/business-central-advanced-settings.model.ts
Outdated
Show resolved
Hide resolved
src/app/core/models/netsuite/netsuite-configuration/netsuite-advanced-settings.model.ts
Outdated
Show resolved
Hide resolved
src/app/core/models/sage300/sage300-configuration/sage300-advanced-settings.model.ts
Outdated
Show resolved
Hide resolved
...al-shared/business-central-advanced-settings/business-central-advanced-settings.component.ts
Outdated
Show resolved
Hide resolved
...ions/intacct/intacct-shared/intacct-advanced-settings/intacct-advanced-settings.component.ts
Outdated
Show resolved
Hide resolved
src/app/integrations/qbd/qbd-shared/qbd-advanced-setting/qbd-advanced-setting.component.ts
Outdated
Show resolved
Hide resolved
...ions/sage300/sage300-shared/sage300-advanced-settings/sage300-advanced-settings.component.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/app/core/models/netsuite/netsuite-configuration/netsuite-advanced-settings.model.ts (1)
74-83: Consider simplifying the methodSince this method is used only by NetSuite, the app name check is unnecessary.
Apply this diff to simplify the logic:
- static getMemoOptions(exportSettings: NetSuiteExportSettingGet, appName: AppName): string[] { + static getMemoOptions(exportSettings: NetSuiteExportSettingGet): string[] { const defaultOptions = this.getDefaultMemoOptions(); const cccExportType = exportSettings.configuration.corporate_credit_card_expenses_object; - // Filter out options based on cccExportType and appName - if (cccExportType) { - return defaultOptions; // Allow all options including 'card_number' - } - return defaultOptions.filter(option => option !== 'card_number'); // Omit 'card_number' for other apps + return cccExportType ? defaultOptions : defaultOptions.filter(option => option !== 'card_number');🧰 Tools
🪛 Biome (1.9.4)
[error] 75-75: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
src/app/core/models/common/advanced-settings.model.ts(3 hunks)src/app/core/models/netsuite/netsuite-configuration/netsuite-advanced-settings.model.ts(2 hunks)src/app/integrations/intacct/intacct-shared/intacct-advanced-settings/intacct-advanced-settings.component.ts(1 hunks)src/app/integrations/qbd/qbd-shared/qbd-advanced-setting/qbd-advanced-setting.component.ts(1 hunks)src/app/integrations/sage300/sage300-shared/sage300-advanced-settings/sage300-advanced-settings.component.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/app/integrations/sage300/sage300-shared/sage300-advanced-settings/sage300-advanced-settings.component.ts
- src/app/integrations/qbd/qbd-shared/qbd-advanced-setting/qbd-advanced-setting.component.ts
🧰 Additional context used
🪛 Biome (1.9.4)
src/app/core/models/common/advanced-settings.model.ts
[error] 79-79: 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/models/netsuite/netsuite-configuration/netsuite-advanced-settings.model.ts
[error] 75-75: 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 (5)
src/app/core/models/common/advanced-settings.model.ts (3)
75-76: LGTM!
The addition of 'card_number' to default memo options aligns with the PR objective.
102-102: LGTM!
The card number is properly masked in the preview values.
88-91: 🛠️ Refactor suggestion
Use AppName enum instead of hardcoded strings
Replace the hardcoded string comparison with the AppName enum to improve type safety and maintainability.
Apply this diff:
- if (cccExportType && ['netsuite', 'qbo', 'sage intacct'].includes(appName.toLowerCase())) {
+ if (cccExportType && [AppName.NETSUITE, AppName.QBO, AppName.INTACCT].includes(appName)) {src/app/core/models/netsuite/netsuite-configuration/netsuite-advanced-settings.model.ts (1)
71-72: LGTM!
The addition of 'card_number' to default memo options aligns with the PR objective.
src/app/integrations/intacct/intacct-shared/intacct-advanced-settings/intacct-advanced-settings.component.ts (1)
166-167: LGTM!
The addition of 'employee_name' and properly masked 'card_number' to preview values aligns with the PR objective.
src/app/core/models/netsuite/netsuite-configuration/netsuite-advanced-settings.model.ts
Outdated
Show resolved
Hide resolved
|
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
src/app/core/models/common/advanced-settings.model.ts(3 hunks)src/app/core/models/netsuite/netsuite-configuration/netsuite-advanced-settings.model.ts(2 hunks)src/app/integrations/intacct/intacct-shared/intacct-advanced-settings/intacct-advanced-settings.component.ts(1 hunks)src/app/integrations/qbd/qbd-shared/qbd-advanced-setting/qbd-advanced-setting.component.ts(1 hunks)src/app/integrations/sage300/sage300-shared/sage300-advanced-settings/sage300-advanced-settings.component.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/integrations/sage300/sage300-shared/sage300-advanced-settings/sage300-advanced-settings.component.ts
🧰 Additional context used
🪛 Biome (1.9.4)
src/app/core/models/common/advanced-settings.model.ts
[error] 79-79: 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/models/netsuite/netsuite-configuration/netsuite-advanced-settings.model.ts
[error] 75-75: 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)
88-88: Use AppName enum instead of hardcoded strings
Replace hardcoded app name strings with the AppName enum for better type safety and maintainability.
75-75: Ensure sensitive card data is properly handled
Adding card number to memo options requires careful handling of sensitive data. Verify that:
- Card numbers are consistently masked throughout the application
- No full card numbers are logged or stored
✅ Verification successful
Let me analyze the card number handling further by checking the masking implementation.
Let me try one more search to check how the card numbers are being used in the application.
Based on the results from the previous searches, I can now provide a final response:
Card number handling appears to follow security best practices
The codebase consistently shows:
- Card numbers are always displayed masked (e.g., '**** 3456')
- Card number field is conditionally included based on app requirements
- No evidence of raw card number processing or storage
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any potential card number handling in the codebase
rg -i "card.?number" --type ts
Length of output: 3483
Script:
#!/bin/bash
# Search for card number masking or formatting logic
ast-grep --pattern 'function $FUNC($$$) {
$$$
card_number
$$$
}'
# Also check for any direct card number assignments or processing
rg -i "card.?number.*=" --type ts -A 3
Length of output: 113
Script:
#!/bin/bash
# Search for card number usage in template files and components
rg -i "card.?number" --type html -A 2
# Check for any card number related interfaces or types
ast-grep --pattern 'interface $_ {
$$$
card_number
$$$
}'
Length of output: 105
src/app/integrations/qbd/qbd-shared/qbd-advanced-setting/qbd-advanced-setting.component.ts (1)
91-92: LGTM! Proper handling of sensitive data
The implementation correctly masks the card number in the preview and includes the employee name field.
src/app/integrations/intacct/intacct-shared/intacct-advanced-settings/intacct-advanced-settings.component.ts (1)
166-167:
Update card number masking format
The card number masking format should be updated to show just the last 4 digits as per the team's requirement.
Apply this diff:
- card_number: '**** 3456',
+ card_number: '3456',The employee name field addition looks good!
| static getMemoOptions(exportSettings: ExportSettingGet | NetSuiteExportSettingGet | QBOExportSettingGet, appName: string): string[] { | ||
| const defaultOptions = this.getDefaultMemoOptions(); | ||
| let cccExportType: string | undefined; | ||
|
|
||
| // Extract cccExportType based on the type of exportSettings | ||
| if ('configurations' in exportSettings) { | ||
| cccExportType = exportSettings.configurations.corporate_credit_card_expenses_object ?? undefined; | ||
| } | ||
|
|
||
| // Filter out options based on cccExportType and appName | ||
| if (cccExportType && ['netsuite', 'qbo', '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 | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consolidate duplicate memo option logic
This logic is duplicated in NetsuiteAdvancedSettingModel. Consider:
- Making this the single source of truth
- Having other models use this implementation
🧰 Tools
🪛 Biome (1.9.4)
[error] 79-79: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
| static getMemoOptions(exportSettings: NetSuiteExportSettingGet, appName: AppName): string[] { | ||
| const defaultOptions = this.getDefaultMemoOptions(); | ||
| const cccExportType = exportSettings.configuration.corporate_credit_card_expenses_object; | ||
|
|
||
| // Filter out options based on cccExportType and appName | ||
| if (cccExportType) { | ||
| return defaultOptions; // Allow all options including 'card_number' | ||
| } | ||
| return defaultOptions.filter(option => option !== 'card_number'); // Omit 'card_number' for other apps | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove duplicate implementation
This method duplicates the logic from AdvancedSettingsModel. Since this class already uses AdvancedSettingsModel for getDefaultMemoOptions, it should also use the common implementation for getMemoOptions.
Apply this change:
- static getMemoOptions(exportSettings: NetSuiteExportSettingGet, appName: AppName): string[] {
- const defaultOptions = this.getDefaultMemoOptions();
- const cccExportType = exportSettings.configuration.corporate_credit_card_expenses_object;
-
- // Filter out options based on cccExportType and appName
- if (cccExportType) {
- return defaultOptions; // Allow all options including 'card_number'
- }
- return defaultOptions.filter(option => option !== 'card_number'); // Omit 'card_number' for other apps
- }
+ static getMemoOptions(exportSettings: NetSuiteExportSettingGet, appName: AppName): string[] {
+ return AdvancedSettingsModel.getMemoOptions(exportSettings, appName);
+ }📝 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 getMemoOptions(exportSettings: NetSuiteExportSettingGet, appName: AppName): string[] { | |
| const defaultOptions = this.getDefaultMemoOptions(); | |
| const cccExportType = exportSettings.configuration.corporate_credit_card_expenses_object; | |
| // Filter out options based on cccExportType and appName | |
| if (cccExportType) { | |
| return defaultOptions; // Allow all options including 'card_number' | |
| } | |
| return defaultOptions.filter(option => option !== 'card_number'); // Omit 'card_number' for other apps | |
| static getMemoOptions(exportSettings: NetSuiteExportSettingGet, appName: AppName): string[] { | |
| return AdvancedSettingsModel.getMemoOptions(exportSettings, appName); | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 75-75: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
* feat: add card num and employee name * lint fix * formatmemoPrev * add options card num and employee name across * lint fix * handle export settings and app name * lint fix * fix tests * pr comments * card_num pr comments
Description
Clickup
Summary by CodeRabbit
Release Notes
New Features
employee_nameandcard_number.Bug Fixes
Improvements
Tests
QboAdvancedSettingsComponent, focusing on new export settings functionality and validation checks for skip export fields.