feat: add split expense grouping config to xero export settings #1071
Conversation
WalkthroughThe changes primarily focus on enhancing the 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: 0
🧹 Outside diff range and nitpick comments (3)
src/app/core/models/xero/xero-configuration/xero-export-settings.model.ts (2)
187-187: Consider adding validation rules for split expense groupingWhile the field is added to validators, there are no specific rules checking if the selected value is valid. Consider adding an
ExportModuleRuleto validate against allowed values fromSplitExpenseGroupingenum.const exportModuleRule: ExportModuleRule[] = [ { formController: 'reimbursableExportType', requiredValue: { } }, { formController: 'creditCardExportType', requiredValue: { } + }, + { + formController: 'splitExpenseGrouping', + requiredValue: { + allowedValues: Object.values(SplitExpenseGrouping) + } } ];
222-222: Consider setting a default value for splitExpenseGrouping controlThe control is initialized without a default value. Consider setting
SplitExpenseGrouping.MULTIPLE_LINE_ITEMas default to match the behavior inconstructPayload.src/app/integrations/xero/xero-shared/xero-export-settings/xero-export-settings.component.html (1)
200-201: Consider enhancing field labels for better accessibility.The current labels could be more descriptive for screen readers.
Consider updating the labels:
- [label]="'How should the split expenses be grouped?'" - [subLabel]="'Choose how expenses should be grouped for split expenses'" + [label]="'Split Expense Grouping Method'" + [subLabel]="'Select how you want to group expenses that have been split across multiple categories or projects'"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/app/core/models/xero/xero-configuration/xero-export-settings.model.ts(5 hunks)src/app/integrations/xero/xero-shared/xero-export-settings/xero-export-settings.component.html(1 hunks)src/app/integrations/xero/xero-shared/xero-export-settings/xero-export-settings.component.ts(3 hunks)
🔇 Additional comments (6)
src/app/integrations/xero/xero-shared/xero-export-settings/xero-export-settings.component.ts (1)
9-9: LGTM! The changes follow Angular best practices.
The additions are well-structured and maintain consistency with the existing codebase:
- Import is properly grouped with related enums
- Properties follow established patterns
- Readonly decorator is correctly used for template exposure
Let's verify the template usage of these new properties:
Also applies to: 52-52, 91-91
src/app/core/models/xero/xero-configuration/xero-export-settings.model.ts (4)
171-182: LGTM! Method follows established patterns
The implementation is consistent with other similar methods in the class and provides clear options for split expense grouping.
233-234: LGTM! Proper default handling
The implementation correctly handles the default case and follows the established payload structure.
221-221: Verify the purpose of searchOption control
The addition of searchOption control seems unrelated to the split expense grouping feature. Please clarify its purpose or consider moving it to a separate PR.
#!/bin/bash
# Search for other occurrences of searchOption
rg "searchOption" --type ts15-16:
Consider making split_expense_grouping optional to maintain backward compatibility
Adding a required property to XeroExpenseGroupSettingPost is a breaking change that could affect existing code. Consider making it optional with ? modifier or providing a default value in the consuming code.
Let's check for existing usages that might break:
src/app/integrations/xero/xero-shared/xero-export-settings/xero-export-settings.component.html (1)
194-207: LGTM! The implementation is consistent with existing patterns.
The new split expense grouping field follows the established patterns:
- Proper conditional rendering based on feature flags
- Consistent styling and spacing
- Clear labeling and field configuration
Let's verify the feature flag implementation:
✅ Verification successful
Feature flag implementation is consistent across integrations
The split expense grouping feature is consistently implemented across Xero, QBO, and Intacct integrations:
- All implementations use the same feature flag path
brandingFeatureConfig.featureFlags.exportSettings.splitExpenseGrouping - Each integration follows similar conditional rendering based on export type
- Consistent field configuration with same labels, mandatory status, and form control name
- Proper component usage (
app-configuration-select-fieldorapp-clone-setting-field) based on context
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify feature flag usage consistency across the codebase
# Look for other usages of splitExpenseGrouping feature flag to ensure consistent implementation
rg -l "exportSettings.splitExpenseGrouping"
Length of output: 429
Script:
#!/bin/bash
# Let's examine how splitExpenseGrouping is used in other files to ensure consistency
rg "splitExpenseGrouping" -A 5 -B 5 src/app/integrations/qbo/qbo-onboarding/qbo-clone-settings/qbo-clone-settings.component.html src/app/integrations/qbo/qbo-shared/qbo-export-settings/qbo-export-settings.component.html src/app/integrations/intacct/intacct-shared/intacct-export-settings/intacct-export-settings.component.html
Length of output: 10965
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/app/integrations/xero/xero-onboarding/xero-clone-settings/xero-clone-settings.component.ts (1)
Line range hint
1-424: Consider adding unit tests for the new functionalityWhile the implementation looks correct, we should ensure proper test coverage for the new split expense grouping feature.
Would you like me to help generate unit tests for:
- Initialization of
splitExpenseGroupingOptions- Feature flag conditional rendering
- Form control binding and validation
src/app/integrations/xero/xero-onboarding/xero-clone-settings/xero-clone-settings.component.html (1)
181-195: LGTM! New split expense grouping field is well-integrated.The implementation follows the existing UI patterns and properly handles conditional rendering based on feature flags and export type.
Consider adding
aria-labelandaria-describedbyattributes to improve accessibility:<app-clone-setting-field [iconSource]="'question-square-outline'" [label]="'How should the split expenses be grouped?'" [options]="splitExpenseGroupingOptions" [placeholder]="'Select split expense grouping'" [form]="exportSettingForm" [isFieldMandatory]="true" [formControllerName]="'splitExpenseGrouping'" [dropdownDisplayKey]="'label'" + [ariaLabel]="'Split expense grouping selection'" + [ariaDescribedBy]="'split-expense-grouping-tooltip'" [tooltipText]="'The selected option will dictate how split expenses will be exported to Xero.'"> </app-clone-setting-field>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/app/integrations/xero/xero-onboarding/xero-clone-settings/xero-clone-settings.component.html(1 hunks)src/app/integrations/xero/xero-onboarding/xero-clone-settings/xero-clone-settings.component.ts(4 hunks)
🔇 Additional comments (3)
src/app/integrations/xero/xero-onboarding/xero-clone-settings/xero-clone-settings.component.ts (2)
5-5: LGTM: Import statements are properly organized
The new imports for brandingFeatureConfig and XeroCorporateCreditCardExpensesObject are correctly added to their respective existing import statements, maintaining code organization.
Also applies to: 11-11
43-44: Verify feature flag usage across the template
The new properties are correctly initialized:
brandingFeatureConfigfor feature flagssplitExpenseGroupingOptionsusing the static method fromXeroExportSettingModelXeroCorporateCreditCardExpensesObjectenum for type safety
However, we should verify that these properties are properly used in the template file.
Also applies to: 65-66, 140-141
✅ Verification successful
Feature flag and properties are correctly used in the template
The verification shows that:
- The feature flag
brandingFeatureConfig.featureFlags.exportSettings.splitExpenseGroupingis properly used in an*ngIfdirective to conditionally render the split expense grouping field - The
splitExpenseGroupingOptionsis correctly bound to theoptionsinput of the clone setting field - The field is only shown when credit card export type is set to
BANK_TRANSACTION - The form control is properly set up with required validation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if brandingFeatureConfig.featureFlags.exportSettings.splitExpenseGrouping is used in the template
# and if splitExpenseGroupingOptions is bound to a form control
# Search for feature flag usage in template
rg -l "brandingFeatureConfig\.featureFlags\.exportSettings\.splitExpenseGrouping" \
--type html \
"src/app/integrations/xero/xero-onboarding/xero-clone-settings/"
# Search for splitExpenseGroupingOptions usage in template
rg -l "splitExpenseGroupingOptions" \
--type html \
"src/app/integrations/xero/xero-onboarding/xero-clone-settings/"
Length of output: 463
Script:
#!/bin/bash
# Let's check the actual usage in the template file
rg -A 5 -B 5 "brandingFeatureConfig\.featureFlags\.exportSettings\.splitExpenseGrouping|splitExpenseGroupingOptions" \
--type html \
"src/app/integrations/xero/xero-onboarding/xero-clone-settings/xero-clone-settings.component.html"
Length of output: 1301
src/app/integrations/xero/xero-onboarding/xero-clone-settings/xero-clone-settings.component.html (1)
181-195: Verify form validation and feature flag configuration.
Please ensure:
- The form validation in the component class properly handles the new
splitExpenseGroupingcontrol - The feature flag
exportSettings.splitExpenseGroupingis properly configured in the environment
✅ Verification successful
Form validation and feature flag integration are properly configured
The verification shows:
- The
splitExpenseGroupingcontrol is properly included in the validation rules for credit card expenses inXeroExportSettingModel.getValidators() - The form validation is handled through the helper service's
addExportSettingFormValidatormethod, which is correctly called in the component - The feature flag is referenced across multiple components (Xero, QBO, Intacct) and branding configurations, indicating proper integration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the feature flag is properly configured
rg -A 5 "exportSettings:\s*{[^}]*splitExpenseGrouping"
# Check if form validation is properly handled in the component
rg "splitExpenseGrouping.*Validators"
Length of output: 242
Script:
#!/bin/bash
# Check feature flag configuration with a simpler pattern
rg -l "splitExpenseGrouping"
# Check form validation in the component file
rg -A 5 "exportSettingForm.*=.*FormGroup" src/app/integrations/xero/xero-onboarding/xero-clone-settings/
# Check the component class for splitExpenseGrouping
ast-grep --pattern 'class XeroCloneSettingsComponent {
$$$
splitExpenseGrouping
$$$
}'
Length of output: 2608
Script:
#!/bin/bash
# Check the feature flag configuration in branding config files
rg -A 5 "exportSettings.*splitExpenseGrouping" src/app/branding/
# Check the form model and validation rules
rg -A 10 "class XeroExportSettingModel" src/app/core/models/xero/xero-configuration/
# Check helper service for form validation
rg -A 5 "addExportSettingFormValidator" src/app/integrations/xero/
Length of output: 3195
Script:
#!/bin/bash
# Check the helper service implementation for form validation
rg -A 10 "class.*HelperService" src/app/integrations/xero/
# Check the model for splitExpenseGrouping
rg -A 5 "splitExpenseGrouping.*interface|interface.*splitExpenseGrouping" src/app/core/models/xero/
# Check the validators implementation
rg -A 10 "getValidators\(\)" src/app/core/models/xero/xero-configuration/xero-export-settings.model.ts
Length of output: 846
JustARatherRidiculouslyLongUsername
merged commit 18d032d
into
xero-split-expense-grouping
Clickup
https://app.clickup.com/t/86cx0vjyk
Summary by CodeRabbit
New Features
Bug Fixes
Documentation