feat: memo structure in xero app #1094
Conversation
WalkthroughThe changes in this pull request involve modifications to configuration objects related to Xero and Fyle integrations. New properties have been added to enhance customization options for exporting memo fields. Updates to the HTML template and component logic improve user interaction with these settings, including a new multi-select component for memo structure customization. The data model has also been updated to accommodate these changes, ensuring that the integration settings are clearer and more functional. 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: 5
🧹 Outside diff range and nitpick comments (4)
src/app/core/models/xero/xero-configuration/xero-advanced-settings.model.ts (1)
115-115: Add validation for memoStructure controlThe
memoStructureform control is added without any validation rules. Consider adding validation to ensure at least one memo field is selected.-memoStructure: new FormControl(advancedSettings.workspace_general_settings.memo_structure), +memoStructure: new FormControl(advancedSettings.workspace_general_settings.memo_structure, [Validators.required, Validators.minLength(1)]),src/app/integrations/xero/xero-shared/xero-advanced-settings/xero-advanced-settings.component.ts (1)
126-134: Extract preview values to a configuration fileThe preview values are hardcoded in the component. Consider moving them to a configuration file for better maintainability.
+// src/app/core/config/memo-preview.config.ts +export const memoPreviewConfig = { + employee_email: '[email protected]', + category: 'Meals and Entertainment', + purpose: 'Client Meeting', + merchant: 'Pizza Hut', + report_number: 'C/2021/12/R/1', + expense_link: `${environment.fyle_app_url}/app/main/#/enterprise/view_expense/` +}; -const previewValues: { [key: string]: string } = { - employee_email: '[email protected]', - category: 'Meals and Entertainment', - purpose: 'Client Meeting', - merchant: 'Pizza Hut', - report_number: 'C/2021/12/R/1', - spent_on: today.toLocaleDateString(), - expense_link: `${environment.fyle_app_url}/app/main/#/enterprise/view_expense/` -}; +const previewValues = { + ...memoPreviewConfig, + spent_on: today.toLocaleDateString() +};src/app/integrations/xero/xero-shared/xero-advanced-settings/xero-advanced-settings.component.html (2)
110-121: Enhance accessibility for multi-select componentAdd ARIA attributes to improve accessibility for screen readers.
<app-configuration-multi-select [form]="advancedSettingForm" [isFieldMandatory]="false" [mandatoryErrorListName]="'Item level description'" [label]="'Set the line item-level Description Field in Xero'" [subLabel]="'You can choose from a list of available data points that you\'d like to export to the description field in Xero.'" [options]="defaultMemoFields" [iconPath]="'list'" [placeholder]="'Set description'" [formControllerName]="'memoStructure'" + [ariaLabel]="'Select memo structure fields'" + [ariaDescribedBy]="'memo-structure-description'" (changeInMultiSelect)="onMultiSelectChange()"> </app-configuration-multi-select>
122-127: Improve preview section handlingAdd handling for empty preview text and loading states.
<div class="preview-text"> <h4 class="tw-text-form-label-text-color tw-mb-12-px">Preview of the Description Field</h4> <div class="preview-box"> - {{memoPreviewText}} + <ng-container *ngIf="memoPreviewText; else emptyPreview"> + {{memoPreviewText}} + </ng-container> + <ng-template #emptyPreview> + <span class="tw-text-gray-500">Select fields to see preview</span> + </ng-template> </div> </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
src/app/branding/c1-contents-config.ts(1 hunks)src/app/branding/fyle-contents-config.ts(1 hunks)src/app/core/models/xero/xero-configuration/xero-advanced-settings.model.ts(3 hunks)src/app/integrations/xero/xero-shared/xero-advanced-settings/xero-advanced-settings.component.html(1 hunks)src/app/integrations/xero/xero-shared/xero-advanced-settings/xero-advanced-settings.component.ts(6 hunks)
| auto_create_merchant_destination_entity: advancedSettingsForm.get('autoCreateMerchantDestinationEntity')?.value, | ||
| memo_structure: advancedSettingsForm.get('memoStructure')?.value |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add default value handling for memo_structure
The payload construction doesn't handle the case where memoStructure control value is undefined or null. Consider adding a default empty array.
-memo_structure: advancedSettingsForm.get('memoStructure')?.value
+memo_structure: advancedSettingsForm.get('memoStructure')?.value || []📝 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.
| auto_create_merchant_destination_entity: advancedSettingsForm.get('autoCreateMerchantDestinationEntity')?.value, | |
| memo_structure: advancedSettingsForm.get('memoStructure')?.value | |
| auto_create_merchant_destination_entity: advancedSettingsForm.get('autoCreateMerchantDestinationEntity')?.value, | |
| memo_structure: advancedSettingsForm.get('memoStructure')?.value || [] |
| onMultiSelectChange() { | ||
| const memo = this.advancedSettingForm.controls.memoStructure.value; | ||
| const changedMemo = AdvancedSettingsModel.formatMemoPreview(memo, this.defaultMemoFields)[1]; | ||
| this.advancedSettingForm.controls.memoStructure.patchValue(changedMemo); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for multi-select changes
The method directly accesses form control values without proper error handling. Consider adding null checks and error handling.
onMultiSelectChange() {
- const memo = this.advancedSettingForm.controls.memoStructure.value;
- const changedMemo = AdvancedSettingsModel.formatMemoPreview(memo, this.defaultMemoFields)[1];
- this.advancedSettingForm.controls.memoStructure.patchValue(changedMemo);
+ try {
+ const memo = this.advancedSettingForm.get('memoStructure')?.value;
+ if (memo) {
+ const changedMemo = AdvancedSettingsModel.formatMemoPreview(memo, this.defaultMemoFields)[1];
+ this.advancedSettingForm.get('memoStructure')?.patchValue(changedMemo);
+ }
+ } catch (error) {
+ console.error('Error updating memo structure:', error);
+ }
}📝 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.
| onMultiSelectChange() { | |
| const memo = this.advancedSettingForm.controls.memoStructure.value; | |
| const changedMemo = AdvancedSettingsModel.formatMemoPreview(memo, this.defaultMemoFields)[1]; | |
| this.advancedSettingForm.controls.memoStructure.patchValue(changedMemo); | |
| } | |
| onMultiSelectChange() { | |
| try { | |
| const memo = this.advancedSettingForm.get('memoStructure')?.value; | |
| if (memo) { | |
| const changedMemo = AdvancedSettingsModel.formatMemoPreview(memo, this.defaultMemoFields)[1]; | |
| this.advancedSettingForm.get('memoStructure')?.patchValue(changedMemo); | |
| } | |
| } catch (error) { | |
| console.error('Error updating memo structure:', error); | |
| } | |
| } |
| private createMemoStructureWatcher(): void { | ||
| this.memoStructure = this.advancedSettingForm.get('memoStructure')?.value; | ||
| this.formatMemoPreview(); | ||
| this.advancedSettingForm.controls.memoStructure.valueChanges.subscribe((memoChanges) => { | ||
| this.memoStructure = memoChanges; | ||
| this.formatMemoPreview(); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Fix potential memory leak in subscription
The subscription to valueChanges is not being unsubscribed, which could lead to memory leaks. Consider implementing OnDestroy and properly managing the subscription.
+private memoStructureSubscription: Subscription;
private createMemoStructureWatcher(): void {
this.memoStructure = this.advancedSettingForm.get('memoStructure')?.value;
this.formatMemoPreview();
- this.advancedSettingForm.controls.memoStructure.valueChanges.subscribe((memoChanges) => {
+ this.memoStructureSubscription = this.advancedSettingForm.get('memoStructure')?.valueChanges.subscribe((memoChanges) => {
this.memoStructure = memoChanges;
this.formatMemoPreview();
});
}
+ngOnDestroy(): void {
+ if (this.memoStructureSubscription) {
+ this.memoStructureSubscription.unsubscribe();
+ }
+}Committable suggestion skipped: line range outside the PR's diff.
| topLevelMemoStructureLabel: 'Customize the Top-Level Memo Field', | ||
| topLevelMemoStructureSubLabel: 'Select the datapoints you\'d like to export to QuickBooks Desktop’s top-level memo field when exporting expenses from Fyle.', | ||
| memoStructureLabel: 'Customize the Line-Item Level Memo Field', | ||
| memoStructureSubLabel: 'Select the datapoints you\'d like to export to QuickBooks Desktop\’s line-item level memo field when exporting expenses from Fyle.', |
There was a problem hiding this comment.
Fix incorrect reference to QuickBooks Desktop in Xero configuration
The labels incorrectly reference QuickBooks Desktop instead of Xero in both the top-level and line-item level memo structure configurations.
Apply this diff to fix the references:
- topLevelMemoStructureLabel: 'Customize the Top-Level Memo Field',
- topLevelMemoStructureSubLabel: 'Select the datapoints you\'d like to export to QuickBooks Desktop's top-level memo field when exporting expenses from Fyle.',
- memoStructureLabel: 'Customize the Line-Item Level Memo Field',
- memoStructureSubLabel: 'Select the datapoints you\'d like to export to QuickBooks Desktop\'s line-item level memo field when exporting expenses from Fyle.',
+ topLevelMemoStructureLabel: 'Customize the Top-Level Memo Field',
+ topLevelMemoStructureSubLabel: 'Select the datapoints you\'d like to export to Xero\'s top-level memo field when exporting expenses from Fyle.',
+ memoStructureLabel: 'Customize the Line-Item Level Memo Field',
+ memoStructureSubLabel: 'Select the datapoints you\'d like to export to Xero\'s line-item level memo field when exporting expenses from Fyle.',📝 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.
| topLevelMemoStructureLabel: 'Customize the Top-Level Memo Field', | |
| topLevelMemoStructureSubLabel: 'Select the datapoints you\'d like to export to QuickBooks Desktop’s top-level memo field when exporting expenses from Fyle.', | |
| memoStructureLabel: 'Customize the Line-Item Level Memo Field', | |
| memoStructureSubLabel: 'Select the datapoints you\'d like to export to QuickBooks Desktop\’s line-item level memo field when exporting expenses from Fyle.', | |
| topLevelMemoStructureLabel: 'Customize the Top-Level Memo Field', | |
| topLevelMemoStructureSubLabel: 'Select the datapoints you\'d like to export to Xero\'s top-level memo field when exporting expenses from Fyle.', | |
| memoStructureLabel: 'Customize the Line-Item Level Memo Field', | |
| memoStructureSubLabel: 'Select the datapoints you\'d like to export to Xero\'s line-item level memo field when exporting expenses from Fyle.', |
| @@ -214,6 +214,8 @@ export const c1Contents = { | |||
| cccExpenseBankAccountLabel: 'Which bank account should the bank transactions post to?', | |||
| cccExpenseStateSubLabel: 'You can choose to only export expenses when they\'ve been labeled approved or closed. ' | |||
| }, | |||
| memoStructureLabel: 'Set the line-item description field in NetSuite', | |||
There was a problem hiding this comment.
instead of netsuite, to make it xero
| @@ -246,6 +246,10 @@ export const fyleContents = { | |||
| billPaymentAccountSubLabel: ', the payment entries will be posted to the selected Payment account in ', | |||
| postEntriesCurrentPeriod: 'Post entries in the current accounting period', | |||
| contentText: 'In this section, you can customize the integration based on your accounting requirements. ', | |||
| topLevelMemoStructureLabel: 'Customize the Top-Level Memo Field', | |||
| topLevelMemoStructureSubLabel: 'Select the datapoints you\'d like to export to QuickBooks Desktop’s top-level memo field when exporting expenses from Fyle.', | |||
Description
memo structure in xero app
Clickup
https://app.clickup.com/tsdfsdfsd/
Summary by CodeRabbit
Release Notes
New Features
Improvements