fix: hide card number and employee name as options for memo field #1123
Conversation
|
Warning Rate limit exceeded@anishfyle has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 0 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe changes introduce an import statement for Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/app/core/models/common/advanced-settings.model.ts(2 hunks)
🔇 Additional comments (2)
src/app/core/models/common/advanced-settings.model.ts (2)
8-8: LGTM!
The import of brandingConfig is correctly added and necessary for the new branding-based condition.
91-94: Verify the impact on dependent components
Let's verify the components that might be affected by this change.
✅ Verification successful
Let me check the actual usage of getMemoOptions in these components to understand the impact better.
Let me check how these components handle the memo options in their templates to ensure they properly handle the card_number option.
Based on the analysis of the components and their templates, I can now provide the final response:
No additional changes required in dependent components
The change in getMemoOptions is safely handled by the existing implementation because:
- All components (Intacct, NetSuite, QBO) simply pass the options to a template-driven dropdown
- The templates use generic bindings (
[options]="defaultMemoOptions") that automatically adapt to the filtered options - No component makes assumptions about specific memo options being present
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for components using getMemoOptions
echo "Components using getMemoOptions:"
rg -l "getMemoOptions" --type ts
# Search for tests of getMemoOptions
echo -e "\nTests for getMemoOptions:"
rg -l "getMemoOptions.*spec" --type ts
# Search for UI templates that might display these fields
echo -e "\nUI templates that might use these fields:"
rg -l "employee_name|card_number" --type html
Length of output: 1130
Script:
#!/bin/bash
# Check how getMemoOptions is used in the components
echo "Usage in Intacct Advanced Settings:"
rg "getMemoOptions" -B 2 -A 2 src/app/integrations/intacct/intacct-shared/intacct-advanced-settings/intacct-advanced-settings.component.ts
echo -e "\nUsage in NetSuite Advanced Settings:"
rg "getMemoOptions" -B 2 -A 2 src/app/integrations/netsuite/netsuite-shared/netsuite-advanced-settings/netsuite-advanced-settings.component.ts
echo -e "\nUsage in QBO Advanced Settings:"
rg "getMemoOptions" -B 2 -A 2 src/app/integrations/qbo/qbo-shared/qbo-advanced-settings/qbo-advanced-settings.component.ts
echo -e "\nUsage in QBO Clone Settings:"
rg "getMemoOptions" -B 2 -A 2 src/app/integrations/qbo/qbo-onboarding/qbo-clone-settings/qbo-clone-settings.component.ts
Length of output: 2307
Script:
#!/bin/bash
# Check template handling of memo options
echo "Intacct Advanced Settings Template:"
rg "defaultMemoFields|defaultMemoOptions" -B 2 -A 2 src/app/integrations/intacct/intacct-shared/intacct-advanced-settings/intacct-advanced-settings.component.html
echo -e "\nNetSuite Advanced Settings Template:"
rg "defaultMemoFields|defaultMemoOptions" -B 2 -A 2 src/app/integrations/netsuite/netsuite-shared/netsuite-advanced-settings/netsuite-advanced-settings.component.html
echo -e "\nQBO Advanced Settings Template:"
rg "defaultMemoFields|defaultMemoOptions" -B 2 -A 2 src/app/integrations/qbo/qbo-shared/qbo-advanced-settings/qbo-advanced-settings.component.html
echo -e "\nQBO Clone Settings Template:"
rg "defaultMemoFields|defaultMemoOptions" -B 2 -A 2 src/app/integrations/qbo/qbo-onboarding/qbo-clone-settings/qbo-clone-settings.component.html
Length of output: 2276
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/app/core/models/common/advanced-settings.model.ts (1)
Line range hint
97-112: Maintain consistency in preview valuesThe
formatMemoPreviewmethod still includes preview values for bothcard_numberandemployee_name. These should be removed to maintain consistency with the hidden memo field options.Apply this diff to remove the preview values:
const previewValues: { [key: string]: string } = { employee_email: '[email protected]', - employee_name: 'John Doe', - card_number: '**** 3456', category: 'Meals and Entertainment', purpose: 'Client Meeting', merchant: 'Pizza Hut', report_number: 'C/2021/12/R/1', spent_on: today.toLocaleDateString(), expense_key: 'E/2024/02/T/11', expense_link: `${environment.fyle_app_url}/app/main/#/enterprise/view_expense/` };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/app/core/models/common/advanced-settings.model.ts(2 hunks)
🔇 Additional comments (2)
src/app/core/models/common/advanced-settings.model.ts (2)
8-8: LGTM!
The import of brandingConfig is correctly added and follows the project's import conventions.
91-91: Verify brandingConfig implementation
Let's verify that the brandingConfig is properly initialized and the brandId property is available.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/app/core/models/common/advanced-settings.model.ts (1)
91-98: Fix style issues and simplify logicThe code has style issues and can be simplified:
- Missing space after
if- Unnecessary
elseafterreturnApply this diff to fix style and improve readability:
- if(brandingConfig.brandId === 'co') { + if (brandingConfig.brandId === 'co') { return defaultOptions.filter(option => option !== 'card_number' && option !== 'employee_name'); - } else { - if (cccExportType && ['netsuite', 'quickbooks online', 'sage intacct'].includes(appName.toLowerCase()) && brandingConfig.brandId === 'fyle') { - return defaultOptions; - } - return defaultOptions.filter(option => option !== 'card_number'); } + + if (cccExportType && + ['netsuite', 'quickbooks online', 'sage intacct'].includes(appName.toLowerCase()) && + brandingConfig.brandId === 'fyle') { + return defaultOptions; + } + + return defaultOptions.filter(option => option !== 'card_number');🧰 Tools
🪛 eslint
[error] 91-91: Expected space(s) after "if".
(keyword-spacing)
[error] 93-98: Unnecessary 'else' after 'return'.
(no-else-return)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/app/core/models/common/advanced-settings.model.ts(2 hunks)
🧰 Additional context used
🪛 eslint
src/app/core/models/common/advanced-settings.model.ts
[error] 91-91: Expected space(s) after "if".
(keyword-spacing)
[error] 93-98: Unnecessary 'else' after 'return'.
(no-else-return)
🔇 Additional comments (2)
src/app/core/models/common/advanced-settings.model.ts (2)
8-8: LGTM!
The import statement for brandingConfig is correctly added and necessary for the new branding-based logic.
91-98:
Implementation doesn't fully meet PR objectives
According to the PR objectives, both card_number and employee_name should be hidden from the memo field options. However, the current implementation only hides both fields for 'co' brand, while only hiding card_number for other cases.
Apply this diff to align with PR objectives:
if (brandingConfig.brandId === 'co') {
return defaultOptions.filter(option => option !== 'card_number' && option !== 'employee_name');
}
if (cccExportType &&
['netsuite', 'quickbooks online', 'sage intacct'].includes(appName.toLowerCase()) &&
brandingConfig.brandId === 'fyle') {
- return defaultOptions;
+ return defaultOptions.filter(option => option !== 'card_number' && option !== 'employee_name');
}
- return defaultOptions.filter(option => option !== 'card_number');
+ return defaultOptions.filter(option => option !== 'card_number' && option !== 'employee_name');Likely invalid or redundant comment.
🧰 Tools
🪛 eslint
[error] 91-91: Expected space(s) after "if".
(keyword-spacing)
[error] 93-98: Unnecessary 'else' after 'return'.
(no-else-return)
|
Description
Clickup
Summary by CodeRabbit
New Features
Bug Fixes