Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Qbd direct je changes #1091

Merged
merged 17 commits into from
Dec 3, 2024
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/app/branding/c1-contents-config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,8 @@ export const c1Contents = {
creditCardExpenseNameinJESubLabel: 'You can select either the \'Merchant Name\' or the \'Employee Name\' to appear in the \'Name\' field of your Journal Entries.',
defaultCCCAccountLabel: 'Set Default Credit Card Account as',
defaultCCCAccountSubLabel: 'Post all your company corporate card transactions to a default credit card account.',
defaultCCCAccountPayableLabel: 'To which accounts payable account should the ',
defaultCCCAccountPayableSubLabel: 'The integration will post the offset credit entry in '
defaultCCCAccountPayableLabel: 'Select the Default Credit Account for adding ',
defaultCCCAccountPayableSubLabel: 'This account will be used for the credit line of the '
Comment on lines +61 to +62
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Truncated labels need to be completed

The search results confirm that the labels in c1-contents-config.ts are incomplete and inconsistent with other configurations. The labels appear to be truncated:

  1. defaultCCCAccountPayableLabel: 'Select the Default Credit Account for adding ' (ends with "for adding")
  2. defaultCCCAccountPayableSubLabel: 'This account will be used for the credit line of the ' (ends with "of the")

Both labels are missing their endings, which are necessary for proper user guidance. This appears to be a bug in the configuration.

🔗 Analysis chain

LGTM! Verify label consistency across integrations.

The updated labels provide better clarity for credit account selection. However, let's ensure consistency across other integration configurations.


Inconsistent credit account labels found across integrations

The search results reveal inconsistencies in credit-related labels across different integrations:

  • QBD Direct: Uses "Select the Default Credit Account for adding" (the modified label)
  • Xero: Uses "The integration will credit the account selected here for corporate credit card expenses exported as journal entries"
  • QBO: Similar inconsistency in terminology

Let's verify the exact labels in the QBD Direct section:


Truncated labels found in QBD Direct configuration

The labels appear to be incomplete as they end abruptly:

  • defaultCCCAccountPayableLabel ends with "for adding"
  • defaultCCCAccountPayableSubLabel ends with "of the"

This seems to be a potential issue with the modified labels. Let's check if similar labels exist in other integration configurations:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for similar credit account labels across different integration configurations
# to ensure consistent terminology.

# Search for credit account related labels
rg -i "credit.*(account|line).*label" --type ts

# Search for similar subLabel patterns
rg -i "credit.*sublabel" --type ts

Length of output: 10755


Script:

#!/bin/bash
# Get more context around the QBD Direct configuration
rg -B5 -A5 "defaultCCCAccountPayableLabel" src/app/branding/c1-contents-config.ts

Length of output: 1050


Script:

#!/bin/bash
# Search for defaultCCCAccountPayable patterns across all files
rg -B2 -A2 "defaultCCCAccount(Payable)?" --type ts

Length of output: 18089

}
},
importSetting: {
Expand Down
2 changes: 1 addition & 1 deletion src/app/branding/fyle-contents-config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ export const fyleContents = {
creditCardExpenseNameinJESubLabel: 'You can select either the \'Merchant Name\' or the \'Employee Name\' to appear in the \'Name\' field of your journal entries.',
defaultCCCAccountLabel: 'Set a Default Credit Card Account',
defaultCCCAccountSubLabel: 'If there\'s no card-to-card mapping or when an expense lacks a corporate card transaction associated with it, expenses will be posted to the default card configured here.',
defaultCCCAccountPayableLabel: 'Select the Default Credit Account for adding Journal Entry ',
defaultCCCAccountPayableLabel: 'Select the Default Credit Account for adding ',
defaultCCCAccountPayableSubLabel: 'This account will be used for the credit line of the '
}
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,8 @@ export class QbdDirectExportSettingModel extends ExportSettingModel {
return form.controls.reimbursableExportType.value === QBDReimbursableExpensesObject.JOURNAL_ENTRY;
case 'nameInJE':
return form.controls.creditCardExportType.value === QBDCorporateCreditCardExpensesObject.JOURNAL_ENTRY;
case 'defaultCreditCardAccountName':
return form.controls.creditCardExportType.value === QBDCorporateCreditCardExpensesObject.CREDIT_CARD_PURCHASE;
default:
return false;
}
Expand All @@ -181,7 +183,7 @@ export class QbdDirectExportSettingModel extends ExportSettingModel {

const exportSettingValidatorRule: ExportSettingValidatorRule = {
reimbursableExpense: ['reimbursableExportType', 'reimbursableExportGroup', 'reimbursableExportDate', 'reimbursableExpenseState', 'employeeMapping'],
creditCardExpense: ['creditCardExportType', 'creditCardExportGroup', 'creditCardExportDate', 'creditCardExpenseState', 'defaultCreditCardAccountName']
creditCardExpense: ['creditCardExportType', 'creditCardExportGroup', 'creditCardExportDate', 'creditCardExpenseState']
};

const exportModuleRule: ExportModuleRule[] = [
Expand All @@ -195,7 +197,7 @@ export class QbdDirectExportSettingModel extends ExportSettingModel {
{
formController: 'creditCardExportType',
requiredValue: {
[QBDCorporateCreditCardExpensesObject.CREDIT_CARD_PURCHASE]: [],
[QBDCorporateCreditCardExpensesObject.CREDIT_CARD_PURCHASE]: ['defaultCreditCardAccountName'],
[QBDCorporateCreditCardExpensesObject.JOURNAL_ENTRY]: ['defaultCCCAccountsPayableAccountName', 'nameInJE']
}
}
Expand Down
8 changes: 6 additions & 2 deletions src/app/core/services/common/mapping.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -170,9 +170,9 @@ export class MappingService {
return this.apiService.post(`/workspaces/${this.workspaceService.getWorkspaceId()}/mappings/`, mapping);
}

getPaginatedDestinationAttributes(attributeType: string, value?: string, display_name?: string, appName?: string): Observable<PaginatedDestinationAttribute> {
getPaginatedDestinationAttributes(attributeType: string, value?: string, display_name?: string, appName?: string, detailed_accout_type?: string[]): Observable<PaginatedDestinationAttribute> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix typo in parameter name

There's a typo in the parameter name: detailed_accout_type should be detailed_account_type.

-  getPaginatedDestinationAttributes(attributeType: string, value?: string, display_name?: string, appName?: string, detailed_accout_type?: string[]): Observable<PaginatedDestinationAttribute> {
+  getPaginatedDestinationAttributes(attributeType: string, value?: string, display_name?: string, appName?: string, detailed_account_type?: string[]): Observable<PaginatedDestinationAttribute> {
📝 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.

Suggested change
getPaginatedDestinationAttributes(attributeType: string, value?: string, display_name?: string, appName?: string, detailed_accout_type?: string[]): Observable<PaginatedDestinationAttribute> {
getPaginatedDestinationAttributes(attributeType: string, value?: string, display_name?: string, appName?: string, detailed_account_type?: string[]): Observable<PaginatedDestinationAttribute> {

const workspaceId = this.workspaceService.getWorkspaceId();
const params: {limit: number, offset: number, attribute_type: string, active?: boolean, value__icontains?: string, value?: string, display_name__in?: string} = {
const params: {limit: number, offset: number, attribute_type: string, active?: boolean, value__icontains?: string, value?: string, display_name__in?: string, detail__account_type__in?: string[]} = {
limit: 100,
offset: 0,
attribute_type: attributeType,
Expand All @@ -191,6 +191,10 @@ export class MappingService {
params.display_name__in = display_name;
}

if (detailed_accout_type) {
params.detail__account_type__in = detailed_accout_type;
}
Comment on lines +194 to +196
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix typo in condition block

The typo in the parameter name needs to be fixed in the condition block as well.

-    if (detailed_accout_type) {
-      params.detail__account_type__in = detailed_accout_type;
+    if (detailed_account_type) {
+      params.detail__account_type__in = detailed_account_type;
📝 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.

Suggested change
if (detailed_accout_type) {
params.detail__account_type__in = detailed_accout_type;
}
if (detailed_account_type) {
params.detail__account_type__in = detailed_account_type;
}


return this.apiService.get(`/workspaces/${workspaceId}/mappings/paginated_destination_attributes/`, params);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,14 @@ import { forkJoin } from 'rxjs';
import { brandingConfig } from 'src/app/branding/c1-contents-config';
import { DestinationAttribute } from 'src/app/core/models/db/destination-attribute.model';
import { MappingSetting } from 'src/app/core/models/db/mapping-setting.model';
import { FyleField, AppName, AccountingField, QBDReimbursableExpensesObject, QBDCorporateCreditCardExpensesObject } from 'src/app/core/models/enum/enum.model';
import { FyleField, AppName, AccountingField, QBDReimbursableExpensesObject, QBDCorporateCreditCardExpensesObject, NameInJournalEntry } from 'src/app/core/models/enum/enum.model';
import { QbdDirectDestinationAttribute } from 'src/app/core/models/qbd-direct/db/qbd-direct-destination-attribuite.model';
import { QbdDirectExportSettingGet } from 'src/app/core/models/qbd-direct/qbd-direct-configuration/qbd-direct-export-settings.model';
import { IntegrationsToastService } from 'src/app/core/services/common/integrations-toast.service';
import { MappingService } from 'src/app/core/services/common/mapping.service';
import { WorkspaceService } from 'src/app/core/services/common/workspace.service';
import { QbdDirectExportSettingsService } from 'src/app/core/services/qbd-direct/qbd-direct-configuration/qbd-direct-export-settings.service';
import { QbdDirectImportSettingsService } from 'src/app/core/services/qbd-direct/qbd-direct-configuration/qbd-direct-import-settings.service';
import { SharedModule } from 'src/app/shared/shared.module';

@Component({
Expand Down Expand Up @@ -46,12 +48,17 @@ export class QbdDirectBaseMappingComponent implements OnInit {

brandingConfig = brandingConfig;

nameInJE: NameInJournalEntry;

chartOfAccounts: string[];

constructor(
private route: ActivatedRoute,
private mappingService: MappingService,
private toastService: IntegrationsToastService,
private workspaceService: WorkspaceService,
private expoerSettingService: QbdDirectExportSettingsService
private exportSettingService: QbdDirectExportSettingsService,
private importSettingService: QbdDirectImportSettingsService
) { }

private getDestinationField(workspaceGeneralSetting: QbdDirectExportSettingGet, mappingSettings: MappingSetting[]): string {
Expand All @@ -64,23 +71,49 @@ export class QbdDirectBaseMappingComponent implements OnInit {
return mappingSettings.find((setting) => setting.source_field === this.sourceField)?.destination_field || '';
}

destinationOptionsWatcher(detailAccountType?: string[]): void {
this.mappingService.getPaginatedDestinationAttributes(this.destinationField, undefined, this.displayName, '', detailAccountType).subscribe((responses) => {
this.destinationOptions = responses.results as QbdDirectDestinationAttribute[];
this.isLoading = false;
});
}
Comment on lines +74 to +79
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add error handling to the destination options API call

The subscription to getPaginatedDestinationAttributes should include error handling to manage API failures gracefully.

  destinationOptionsWatcher(detailAccountType?: string[]): void {
    this.mappingService.getPaginatedDestinationAttributes(this.destinationField, undefined, this.displayName, '', detailAccountType).subscribe((responses) => {
      this.destinationOptions = responses.results as QbdDirectDestinationAttribute[];
      this.isLoading = false;
-    });
+    }, (error) => {
+      this.isLoading = false;
+      this.toastService.show('Error fetching destination options', 'danger');
+      console.error('Error fetching destination options:', error);
+    });
  }

Committable suggestion skipped: line range outside the PR's diff.


cccAccpuntOptions(): void {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getCCCAccountOptions()

if (this.cccExpenseObject === QBDCorporateCreditCardExpensesObject.CREDIT_CARD_PURCHASE) {
this.destinationOptionsWatcher(['CreditCard']);
} else if (this.cccExpenseObject === QBDCorporateCreditCardExpensesObject.JOURNAL_ENTRY && this.employeeFieldMapping === FyleField.EMPLOYEE && this.nameInJE === NameInJournalEntry.EMPLOYEE) {
this.destinationOptionsWatcher(['Bank', 'CreditCard', 'OtherCurrentLiability', 'LongTermLiability']);
} else {
this.destinationOptionsWatcher(['Bank', 'AccountsPayable', 'CreditCard', 'OtherCurrentLiability', 'LongTermLiability']);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Fix typo in method name and simplify conditional logic

The method name has a typo, and the conditional logic could be simplified for better maintainability.

- cccAccpuntOptions(): void {
+ cccAccountOptions(): void {
+   const accountTypes = ['CreditCard'];
    if (this.cccExpenseObject === QBDCorporateCreditCardExpensesObject.CREDIT_CARD_PURCHASE) {
-     this.destinationOptionsWatcher(['CreditCard']);
+     this.destinationOptionsWatcher(accountTypes);
    } else if (this.cccExpenseObject === QBDCorporateCreditCardExpensesObject.JOURNAL_ENTRY && 
               this.employeeFieldMapping === FyleField.EMPLOYEE && 
               this.nameInJE === NameInJournalEntry.EMPLOYEE) {
-     this.destinationOptionsWatcher(['Bank', 'CreditCard', 'OtherCurrentLiability', 'LongTermLiability']);
+     accountTypes.push('Bank', 'OtherCurrentLiability', 'LongTermLiability');
+     this.destinationOptionsWatcher(accountTypes);
    } else {
-     this.destinationOptionsWatcher(['Bank', 'AccountsPayable', 'CreditCard', 'OtherCurrentLiability', 'LongTermLiability']);
+     accountTypes.push('Bank', 'AccountsPayable', 'OtherCurrentLiability', 'LongTermLiability');
+     this.destinationOptionsWatcher(accountTypes);
    }
  }
📝 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.

Suggested change
cccAccpuntOptions(): void {
if (this.cccExpenseObject === QBDCorporateCreditCardExpensesObject.CREDIT_CARD_PURCHASE) {
this.destinationOptionsWatcher(['CreditCard']);
} else if (this.cccExpenseObject === QBDCorporateCreditCardExpensesObject.JOURNAL_ENTRY && this.employeeFieldMapping === FyleField.EMPLOYEE && this.nameInJE === NameInJournalEntry.EMPLOYEE) {
this.destinationOptionsWatcher(['Bank', 'CreditCard', 'OtherCurrentLiability', 'LongTermLiability']);
} else {
this.destinationOptionsWatcher(['Bank', 'AccountsPayable', 'CreditCard', 'OtherCurrentLiability', 'LongTermLiability']);
}
}
cccAccountOptions(): void {
const accountTypes = ['CreditCard'];
if (this.cccExpenseObject === QBDCorporateCreditCardExpensesObject.CREDIT_CARD_PURCHASE) {
this.destinationOptionsWatcher(accountTypes);
} else if (this.cccExpenseObject === QBDCorporateCreditCardExpensesObject.JOURNAL_ENTRY && this.employeeFieldMapping === FyleField.EMPLOYEE && this.nameInJE === NameInJournalEntry.EMPLOYEE) {
accountTypes.push('Bank', 'OtherCurrentLiability', 'LongTermLiability');
this.destinationOptionsWatcher(accountTypes);
} else {
accountTypes.push('Bank', 'AccountsPayable', 'OtherCurrentLiability', 'LongTermLiability');
this.destinationOptionsWatcher(accountTypes);
}
}


categoryOptions(): void {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getAccountOptions()

this.destinationOptionsWatcher(this.chartOfAccounts);
}

private setupPage(): void {
this.sourceField = decodeURIComponent(this.route.snapshot.params.source_field.toUpperCase());
forkJoin([
this.expoerSettingService.getQbdExportSettings(),
this.exportSettingService.getQbdExportSettings(),
this.importSettingService.getImportSettings(),
this.mappingService.getMappingSettings()
]).subscribe((responses) => {
this.reimbursableExpenseObject = responses[0].reimbursable_expense_export_type;
this.cccExpenseObject = responses[0].credit_card_expense_export_type;
this.employeeFieldMapping = (responses[0].employee_field_mapping as unknown as FyleField);

this.destinationField = this.getDestinationField(responses[0], responses[1].results);


this.mappingService.getPaginatedDestinationAttributes(this.destinationField, undefined, this.displayName).subscribe((responses) => {
this.destinationOptions = responses.results;
this.isLoading = false;
});
this.nameInJE = responses[0].name_in_journal_entry;
this.chartOfAccounts = responses[1].import_settings.chart_of_accounts;

this.destinationField = this.getDestinationField(responses[0], responses[2].results);

if (this.sourceField === 'CORPORATE_CARD') {
this.cccAccpuntOptions();
} else if (this.sourceField === 'CATEGORY') {
this.categoryOptions();
} else {
this.destinationOptionsWatcher();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve error handling and method organization in setupPage

The setupPage method could benefit from better error handling and separation of concerns.

  private setupPage(): void {
    this.sourceField = decodeURIComponent(this.route.snapshot.params.source_field.toUpperCase());
    forkJoin([
      this.exportSettingService.getQbdExportSettings(),
      this.importSettingService.getImportSettings(),
      this.mappingService.getMappingSettings()
-   ]).subscribe((responses) => {
+   ]).subscribe({
+     next: (responses) => {
      this.reimbursableExpenseObject = responses[0].reimbursable_expense_export_type;
      this.cccExpenseObject = responses[0].credit_card_expense_export_type;
      this.employeeFieldMapping = (responses[0].employee_field_mapping as unknown as FyleField);
      this.nameInJE = responses[0].name_in_journal_entry;
      this.chartOfAccounts = responses[1].import_settings.chart_of_accounts;

      this.destinationField = this.getDestinationField(responses[0], responses[2].results);

+     this.updateDestinationOptions();
+   },
+     error: (error) => {
+       this.isLoading = false;
+       this.toastService.show('Error loading mapping settings', 'danger');
+       console.error('Error in setupPage:', error);
+     }
    });
  }

+ private updateDestinationOptions(): void {
    if (this.sourceField === 'CORPORATE_CARD') {
-     this.cccAccpuntOptions();
+     this.cccAccountOptions();
    } else if (this.sourceField === 'CATEGORY') {
      this.categoryOptions();
    } else {
      this.destinationOptionsWatcher();
    }
+ }

Committable suggestion skipped: line range outside the PR's diff.

});
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@
<div *ngIf="helperService.isFieldRequired(exportSettingsForm, 'defaultReimbursableAccountsPayableAccountName')">
<app-configuration-select-field
[form]="exportSettingsForm"
[destinationAttributes]="destinationOptionsWatcher(['AccountsPayable'] , destinationAccounts)"
[destinationAttributes]="reimbursableAccpuntOptions()"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Fix typo in method name and update implementation

The method name reimbursableAccpuntOptions() contains a typo and should be renamed to reimbursableAccountOptions(). The implementation itself is correct as it properly filters destination accounts based on account types for reimbursable expenses.

  • Line 60: Rename reimbursableAccpuntOptions() to reimbursableAccountOptions()
🔗 Analysis chain

Fix typo in method name and verify implementation

The method name contains a typo: reimbursableAccpuntOptions should be reimbursableAccountOptions.

Please verify that the new method correctly handles all account filtering scenarios. Run this script to check the implementation:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Find the implementation of reimbursableAccpuntOptions method
# and any related test cases

ast-grep --pattern 'reimbursableAccpuntOptions()' -A 10
rg -l "reimbursableAccpuntOptions" --type spec

Length of output: 253


Script:

#!/bin/bash
# Let's try different approaches to find the implementation:

# 1. Search for the method name with potential typos using ripgrep
rg -i "reimbursable.*options" -A 5

# 2. Search for the component class file to find the implementation
fd -e ts "qbd-direct-export-settings.component.ts$"

# 3. If found, let's examine the component file
if [ -f "src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-export-settings/qbd-direct-export-settings.component.ts" ]; then
    cat "src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-export-settings/qbd-direct-export-settings.component.ts"
fi

Length of output: 99845

[destinationOptionKey]="QbdDirectExportSettingDestinationOptionKey.ACCOUNTS_PAYABLE"
[isOptionSearchInProgress]="isOptionSearchInProgress"
[isAdvanceSearchEnabled]="true"
Expand Down Expand Up @@ -156,6 +156,21 @@
</app-configuration-select-field>
</div>

<div *ngIf="brandingFeatureConfig.featureFlags.exportSettings.nameInJournalEntry && exportSettingsForm.value.creditCardExportType === QBDCorporateCreditCardExpensesObject.JOURNAL_ENTRY">
<app-configuration-select-field
[form]="exportSettingsForm"
[isFieldMandatory]="true"
[showClearIcon]="true"
[mandatoryErrorListName]="'Name in which Journal Entry should export'"
[label]="brandingContent.corporateCard.creditCardExpenseNameinJELabel"
[subLabel]="brandingContent.corporateCard.creditCardExpenseNameinJESubLabel"
[options]="nameInJEOptions"
[iconPath]="'list'"
[placeholder]="'Select a name'"
[formControllerName]="'nameInJE'">
</app-configuration-select-field>
</div>

<div *ngIf="exportSettingsForm.get('creditCardExportType')?.value && helperService.isFieldRequired(exportSettingsForm, 'defaultCreditCardAccountName')">
<app-configuration-select-field
[form]="exportSettingsForm"
Expand Down Expand Up @@ -196,20 +211,6 @@
</app-configuration-select-field>
</div>

<div *ngIf="brandingFeatureConfig.featureFlags.exportSettings.nameInJournalEntry && exportSettingsForm.value.creditCardExportType === QBDCorporateCreditCardExpensesObject.JOURNAL_ENTRY">
<app-configuration-select-field
[form]="exportSettingsForm"
[isFieldMandatory]="true"
[showClearIcon]="true"
[mandatoryErrorListName]="'Name in which Journal Entry should export'"
[label]="brandingContent.corporateCard.creditCardExpenseNameinJELabel"
[subLabel]="brandingContent.corporateCard.creditCardExpenseNameinJESubLabel"
[options]="nameInJEOptions"
[iconPath]="'list'"
[placeholder]="'Select a name'"
[formControllerName]="'nameInJE'">
</app-configuration-select-field>
</div>
</div>
</div>

Expand Down Expand Up @@ -238,7 +239,7 @@
[iconPath]="'question-square-outline'"
[placeholder]="'Select expense grouping'"
[isDisableTextRequired]="true"
[isDisabled]="exportSettingsForm.get('creditCardExportType')?.value === QBDCorporateCreditCardExpensesObject.CREDIT_CARD_PURCHASE ? true : false"
[isDisabled]="exportSettingsForm.get('creditCardExportType')?.value === QBDCorporateCreditCardExpensesObject.CREDIT_CARD_PURCHASE || exportSettingsForm.get('creditCardExportType')?.value === QBDCorporateCreditCardExpensesObject.JOURNAL_ENTRY ? true : false"
[formControllerName]="'creditCardExportGroup'">
</app-configuration-select-field>
</div>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -101,9 +101,18 @@ export class QbdDirectExportSettingsComponent implements OnInit{
return true;
}

reimbursableAccpuntOptions(): DestinationAttribute[] {
if (this.exportSettingsForm.controls.employeeMapping.value === EmployeeFieldMapping.EMPLOYEE) {
return this.destinationOptionsWatcher(['Bank', 'CreditCard', 'OtherCurrentLiability', 'LongTermLiability'], this.destinationAccounts);
}
return this.destinationOptionsWatcher(['Bank', 'AccountsPayable', 'CreditCard', 'OtherCurrentLiability', 'LongTermLiability'], this.destinationAccounts);
}
Comment on lines +104 to +109
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Fix typo and improve robustness of reimbursableAccpuntOptions method

Several improvements needed:

  1. Fix typo in method name ("Accpunt" → "Account")
  2. Add null check for destinationAccounts
  3. Extract account types as readonly constants
  4. Consider making method private if only used in template
+ private readonly EMPLOYEE_ACCOUNT_TYPES = ['Bank', 'CreditCard', 'OtherCurrentLiability', 'LongTermLiability'] as const;
+ private readonly ALL_ACCOUNT_TYPES = ['Bank', 'AccountsPayable', 'CreditCard', 'OtherCurrentLiability', 'LongTermLiability'] as const;

- reimbursableAccpuntOptions(): DestinationAttribute[] {
+ private reimbursableAccountOptions(): DestinationAttribute[] {
+   if (!this.destinationAccounts) {
+     return [];
+   }
    if (this.exportSettingsForm.controls.employeeMapping.value === EmployeeFieldMapping.EMPLOYEE) {
-     return this.destinationOptionsWatcher(['Bank', 'CreditCard', 'OtherCurrentLiability', 'LongTermLiability'], this.destinationAccounts);
+     return this.destinationOptionsWatcher(this.EMPLOYEE_ACCOUNT_TYPES, this.destinationAccounts);
    }
-   return this.destinationOptionsWatcher(['Bank', 'AccountsPayable', 'CreditCard', 'OtherCurrentLiability', 'LongTermLiability'], this.destinationAccounts);
+   return this.destinationOptionsWatcher(this.ALL_ACCOUNT_TYPES, this.destinationAccounts);
  }
📝 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.

Suggested change
reimbursableAccpuntOptions(): DestinationAttribute[] {
if (this.exportSettingsForm.controls.employeeMapping.value === EmployeeFieldMapping.EMPLOYEE) {
return this.destinationOptionsWatcher(['Bank', 'CreditCard', 'OtherCurrentLiability', 'LongTermLiability'], this.destinationAccounts);
}
return this.destinationOptionsWatcher(['Bank', 'AccountsPayable', 'CreditCard', 'OtherCurrentLiability', 'LongTermLiability'], this.destinationAccounts);
}
private readonly EMPLOYEE_ACCOUNT_TYPES = ['Bank', 'CreditCard', 'OtherCurrentLiability', 'LongTermLiability'] as const;
private readonly ALL_ACCOUNT_TYPES = ['Bank', 'AccountsPayable', 'CreditCard', 'OtherCurrentLiability', 'LongTermLiability'] as const;
private reimbursableAccountOptions(): DestinationAttribute[] {
if (!this.destinationAccounts) {
return [];
}
if (this.exportSettingsForm.controls.employeeMapping.value === EmployeeFieldMapping.EMPLOYEE) {
return this.destinationOptionsWatcher(this.EMPLOYEE_ACCOUNT_TYPES, this.destinationAccounts);
}
return this.destinationOptionsWatcher(this.ALL_ACCOUNT_TYPES, this.destinationAccounts);
}


cccAccpuntOptions(cccExportType: string): DestinationAttribute[] {
if (cccExportType === QBDCorporateCreditCardExpensesObject.CREDIT_CARD_PURCHASE) {
return this.destinationOptionsWatcher(['CreditCard'], this.destinationAccounts);
} else if (cccExportType === QBDCorporateCreditCardExpensesObject.JOURNAL_ENTRY && this.exportSettingsForm.controls.employeeMapping.value === EmployeeFieldMapping.EMPLOYEE && this.exportSettingsForm.controls.nameInJE.value === EmployeeFieldMapping.EMPLOYEE) {
return this.destinationOptionsWatcher(['Bank', 'CreditCard', 'OtherCurrentLiability', 'LongTermLiability'], this.destinationAccounts);
}
return this.destinationOptionsWatcher(['Bank', 'AccountsPayable', 'CreditCard', 'OtherCurrentLiability', 'LongTermLiability'], this.destinationAccounts);
Comment on lines 111 to 117
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Refactor cccAccpuntOptions for better maintainability

The method needs several improvements:

  1. Fix typo in method name
  2. Simplify complex conditional logic
  3. Reuse account type constants
  4. Add null check for destinationAccounts
+ private readonly CREDIT_CARD_ACCOUNT_TYPES = ['CreditCard'] as const;

- cccAccpuntOptions(cccExportType: string): DestinationAttribute[] {
+ private cccAccountOptions(cccExportType: string): DestinationAttribute[] {
+   if (!this.destinationAccounts) {
+     return [];
+   }

    if (cccExportType === QBDCorporateCreditCardExpensesObject.CREDIT_CARD_PURCHASE) {
-     return this.destinationOptionsWatcher(['CreditCard'], this.destinationAccounts);
+     return this.destinationOptionsWatcher(this.CREDIT_CARD_ACCOUNT_TYPES, this.destinationAccounts);
    }

+   const isJournalEntryWithEmployee = cccExportType === QBDCorporateCreditCardExpensesObject.JOURNAL_ENTRY 
+     && this.exportSettingsForm.controls.employeeMapping.value === EmployeeFieldMapping.EMPLOYEE 
+     && this.exportSettingsForm.controls.nameInJE.value === EmployeeFieldMapping.EMPLOYEE;

-   if (cccExportType === QBDCorporateCreditCardExpensesObject.JOURNAL_ENTRY && this.exportSettingsForm.controls.employeeMapping.value === EmployeeFieldMapping.EMPLOYEE && this.exportSettingsForm.controls.nameInJE.value === EmployeeFieldMapping.EMPLOYEE) {
+   if (isJournalEntryWithEmployee) {
-     return this.destinationOptionsWatcher(['Bank', 'CreditCard', 'OtherCurrentLiability', 'LongTermLiability'], this.destinationAccounts);
+     return this.destinationOptionsWatcher(this.EMPLOYEE_ACCOUNT_TYPES, this.destinationAccounts);
    }

-   return this.destinationOptionsWatcher(['Bank', 'AccountsPayable', 'CreditCard', 'OtherCurrentLiability', 'LongTermLiability'], this.destinationAccounts);
+   return this.destinationOptionsWatcher(this.ALL_ACCOUNT_TYPES, this.destinationAccounts);
  }

Committable suggestion skipped: line range outside the PR's diff.

}
Expand Down Expand Up @@ -173,14 +182,9 @@ export class QbdDirectExportSettingsComponent implements OnInit{

cccExportTypeWatcher(): void {
this.exportSettingsForm.controls.creditCardExportType.valueChanges.subscribe((creditCardExportTypeValue) => {
if (creditCardExportTypeValue === QBDCorporateCreditCardExpensesObject.CREDIT_CARD_PURCHASE) {
this.exportSettingsForm.controls.creditCardExportGroup.patchValue(this.creditCardExpenseGroupingFieldOptions[1].value);
this.exportSettingsForm.controls.creditCardExportGroup.disable();
this.creditCardExpenseGroupingFieldOptions = [QbdDirectExportSettingModel.expenseGroupingFieldOptions()[1]];
} else {
this.exportSettingsForm.controls.creditCardExportGroup.enable();
this.creditCardExpenseGroupingFieldOptions = QbdDirectExportSettingModel.expenseGroupingFieldOptions();
}
});
}

Expand Down