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.

Sign up for GitHub

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

fix: Filter extracted merchant name before display #3214

Merged
merged 8 commits into from
Sep 30, 2024
Merged

fix: Filter extracted merchant name before display #3214

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
7f2cb0f
add loop
harshal015 Sep 22, 2024
9a2ebce
refactor
harshal015 Sep 23, 2024
baf5bac
replace at more places
harshal015 Sep 23, 2024
4026d19
some fixes
harshal015 Sep 26, 2024
00fbab4
minor
harshal015 Sep 26, 2024
aea0af9
Merge branch 'master' into FYLE-86cwdt5t1
harshal015 Sep 29, 2024
394bed5
fix unit tests
harshal015 Sep 30, 2024
63b0bd6
minor
harshal015 Sep 30, 2024
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
1 change: 1 addition & 0 deletions src/app/fyle/add-edit-expense/add-edit-expense-3.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,7 @@ export function TestCases3(getTestBed) {
component._isExpandedView = true;
component.navigateBack = true;
component.hardwareBackButtonAction = new Subscription();
component.vendorOptions = ['vendor'];
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Avoid hardcoding vendor options in the test setup

Hardcoding component.vendorOptions to ['vendor'] may limit the test coverage and flexibility. Consider using a mock or fixture that provides a realistic set of vendor options for more comprehensive testing.

fixture.detectChanges();
});

Expand Down
2 changes: 1 addition & 1 deletion src/app/fyle/add-edit-expense/add-edit-expense.page.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -921,7 +921,7 @@
</app-virtual-select>
<div
class="add-edit-expense--auto-coded"
*ngIf="(autoCodedData?.vendor_name && fg.controls.vendor_id?.value?.display_name) && autoCodedData.vendor_name === fg.controls.vendor_id.value.display_name"
*ngIf="(autoCodedData?.vendor_name && fg.controls.vendor_id?.value?.display_name) && autoCodedData.vendor_name?.toLowerCase() === fg.controls.vendor_id.value.display_name?.toLowerCase()"
>
<mat-icon class="add-edit-expense--sparkle-icon" svgIcon="sparkle"></mat-icon>
Merchant is auto coded.
Expand Down
17 changes: 14 additions & 3 deletions src/app/fyle/add-edit-expense/add-edit-expense.page.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -439,6 +439,8 @@

selectedCategory$: Observable<OrgCategory>;

vendorOptions: string[];
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Initialize 'vendorOptions' to an Empty Array

To prevent vendorOptions from being undefined, initialize it to an empty array. This ensures that vendorOptions is always defined, avoiding potential issues when filterVendor is called.

Apply this diff to initialize vendorOptions:

-  vendorOptions: string[];
+  vendorOptions: string[] = [];
📝 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
vendorOptions: string[];
vendorOptions: string[] = [];


constructor(
private activatedRoute: ActivatedRoute,
private accountsService: AccountsService,
Expand Down Expand Up @@ -1485,7 +1487,8 @@
}

if (extractedData.vendor) {
etxn.tx.vendor = extractedData.vendor;
const vendor = this.filterVendor(extractedData.vendor);
etxn.tx.vendor = vendor;
Comment on lines +1490 to +1491
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Handle Null Values Returned from 'filterVendor'

The filterVendor method may return null if the extracted vendor is not found in vendorOptions. Assigning null to etxn.tx.vendor may lead to unintended behavior if etxn.tx.vendor is expected to be a non-null string elsewhere. Add a null check before assigning to ensure robustness.

Apply this diff to add a null check:

               const vendor = this.filterVendor(extractedData.vendor);
-              etxn.tx.vendor = vendor;
+              if (vendor) {
+                etxn.tx.vendor = vendor;
+              }
📝 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
const vendor = this.filterVendor(extractedData.vendor);
etxn.tx.vendor = vendor;
const vendor = this.filterVendor(extractedData.vendor);
if (vendor) {
etxn.tx.vendor = vendor;
}

}

if (extractedCategory) {
Expand Down Expand Up @@ -2434,6 +2437,7 @@
const ifOptions = expenseField.options && expenseField.options.length > 0;
if (ifOptions) {
if (tfc === 'vendor_id') {
this.vendorOptions = options;
expenseField.options = options.map((value) => ({
label: value,
value: {
Expand Down Expand Up @@ -2724,7 +2728,8 @@
}

if (etxn.tx.extracted_data.vendor && !etxn.tx.vendor) {
etxn.tx.vendor = etxn.tx.extracted_data.vendor;
const vendor = this.filterVendor(etxn.tx.extracted_data.vendor);
etxn.tx.vendor = vendor;
}

if (
Expand Down Expand Up @@ -4525,8 +4530,10 @@
}

if (!this.fg.controls.vendor_id.value && extractedData.vendor) {
const vendor = this.filterVendor(extractedData.vendor);

this.fg.patchValue({
vendor_id: { display_name: extractedData.vendor },
vendor_id: { display_name: vendor },
Comment on lines +4533 to +4536
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Handle Potential Null Value When Setting 'vendor_id.display_name'

The vendor variable may be null. Assigning null to display_name may cause issues if display_name is expected to be a non-null string. Add a null check before assignment.

Apply this diff to add a null check:

              const vendor = this.filterVendor(extractedData.vendor);
+             if (vendor) {
                vendor_id: { display_name: vendor },
+             }

Alternatively, handle the case when vendor is null appropriately.

Committable suggestion was skipped due to low confidence.

});
}

Expand All @@ -4548,7 +4555,11 @@
});
}

private filterVendor(vendor: string): string | null {
return this.vendorOptions?.find((option) => option.toLowerCase() === vendor.toLowerCase()) || null;
}
Comment on lines +4558 to +4560
Copy link

@coderabbitai coderabbitai bot Sep 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue

Handle null or undefined 'vendor' in 'filterVendor' method.

The filterVendor method assumes that vendor is a non-null string. If vendor is null or undefined, calling vendor.toLowerCase() will result in a runtime error. Please add a null check to handle such cases.

Apply this diff to add the null check:

- private filterVendor(vendor: string): string | null {
+ private filterVendor(vendor?: string): string | null {
+   if (!vendor) {
+     return null;
+   }
    return this.vendorOptions?.find((option) => option.toLowerCase() === vendor.toLowerCase()) || null;
  }
📝 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
private filterVendor(vendor: string): string | null {
return this.vendorOptions?.find((option) => option.toLowerCase() === vendor.toLowerCase()) || null;
}
private filterVendor(vendor?: string): string | null {
if (!vendor) {
return null;
}
return this.vendorOptions?.find((option) => option.toLowerCase() === vendor.toLowerCase()) || null;
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this wont be required as we already have the check where its being called

Copy link

Choose a reason for hiding this comment

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

@harshal015, understood. Thank you for clarifying that vendor is always checked before calling filterVendor.

✏️ Learnings added 
Learnt from: harshal015
PR: fylein/fyle-mobile-app#3214
File: src/app/fyle/add-edit-expense/add-edit-expense.page.ts:4558-4560
Timestamp: 2024-09-26T20:56:20.358Z
Learning: In `AddEditExpensePage`, the `vendor` parameter passed to `filterVendor` is always non-null because it is validated before calling the method.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


attachReceipts(data: { type: string; dataUrl: string | ArrayBuffer; actionSource?: string }): void {

Check failure on line 4562 in src/app/fyle/add-edit-expense/add-edit-expense.page.ts

View workflow job for this annotation

GitHub Actions / Run linters 

Member attachReceipts should be declared before all private instance method definitions

Check failure on line 4562 in src/app/fyle/add-edit-expense/add-edit-expense.page.ts

View workflow job for this annotation

GitHub Actions / Run linters 

Member attachReceipts should be declared before all private instance method definitions
if (data) {
const fileInfo = {
type: data.type,
Expand Down Expand Up @@ -4656,7 +4667,7 @@
}
}

async uploadFileCallback(file: File): Promise<void> {

Check failure on line 4670 in src/app/fyle/add-edit-expense/add-edit-expense.page.ts

View workflow job for this annotation

GitHub Actions / Run linters 

Member uploadFileCallback should be declared before all private instance method definitions

Check failure on line 4670 in src/app/fyle/add-edit-expense/add-edit-expense.page.ts

View workflow job for this annotation

GitHub Actions / Run linters 

Member uploadFileCallback should be declared before all private instance method definitions
let fileData: { type: string; dataUrl: string | ArrayBuffer; actionSource: string };
if (file) {
if (file.size < MAX_FILE_SIZE) {
Expand All @@ -4674,12 +4685,12 @@
}
}

async onChangeCallback(nativeElement: HTMLInputElement): Promise<void> {

Check failure on line 4688 in src/app/fyle/add-edit-expense/add-edit-expense.page.ts

View workflow job for this annotation

GitHub Actions / Run linters 

Member onChangeCallback should be declared before all private instance method definitions

Check failure on line 4688 in src/app/fyle/add-edit-expense/add-edit-expense.page.ts

View workflow job for this annotation

GitHub Actions / Run linters 

Member onChangeCallback should be declared before all private instance method definitions
const file = nativeElement.files[0];
this.uploadFileCallback(file);
}

async addAttachments(event: Event): Promise<void> {

Check failure on line 4693 in src/app/fyle/add-edit-expense/add-edit-expense.page.ts

View workflow job for this annotation

GitHub Actions / Run linters 

Member addAttachments should be declared before all private instance method definitions

Check failure on line 4693 in src/app/fyle/add-edit-expense/add-edit-expense.page.ts

View workflow job for this annotation

GitHub Actions / Run linters 

Member addAttachments should be declared before all private instance method definitions
event.stopPropagation();

if (this.platform.is('ios')) {
Expand Down Expand Up @@ -4749,7 +4760,7 @@
}
}

getReceiptExtension(name: string): string | null {

Check failure on line 4763 in src/app/fyle/add-edit-expense/add-edit-expense.page.ts

View workflow job for this annotation

GitHub Actions / Run linters 

Member getReceiptExtension should be declared before all private instance method definitions

Check failure on line 4763 in src/app/fyle/add-edit-expense/add-edit-expense.page.ts

View workflow job for this annotation

GitHub Actions / Run linters 

Member getReceiptExtension should be declared before all private instance method definitions
let res: string = null;

if (name) {
Expand All @@ -4764,7 +4775,7 @@
return res;
}

getReceiptDetails(file: FileObject): { type: string; thumbnail: string } {

Check failure on line 4778 in src/app/fyle/add-edit-expense/add-edit-expense.page.ts

View workflow job for this annotation

GitHub Actions / Run linters 

Member getReceiptDetails should be declared before all private instance method definitions

Check failure on line 4778 in src/app/fyle/add-edit-expense/add-edit-expense.page.ts

View workflow job for this annotation

GitHub Actions / Run linters 

Member getReceiptDetails should be declared before all private instance method definitions
const ext = this.getReceiptExtension(file.name);
const res = {
type: 'unknown',
Expand All @@ -4782,7 +4793,7 @@
return res;
}

viewAttachments(): void {

Check failure on line 4796 in src/app/fyle/add-edit-expense/add-edit-expense.page.ts

View workflow job for this annotation

GitHub Actions / Run linters 

Member viewAttachments should be declared before all private instance method definitions

Check failure on line 4796 in src/app/fyle/add-edit-expense/add-edit-expense.page.ts

View workflow job for this annotation

GitHub Actions / Run linters 

Member viewAttachments should be declared before all private instance method definitions
const attachements$ = this.getExpenseAttachments(this.mode);

from(this.loaderService.showLoader())
Expand Down Expand Up @@ -4838,7 +4849,7 @@
});
}

getDeleteReportParams(

Check failure on line 4852 in src/app/fyle/add-edit-expense/add-edit-expense.page.ts

View workflow job for this annotation

GitHub Actions / Run linters 

Member getDeleteReportParams should be declared before all private instance method definitions

Check failure on line 4852 in src/app/fyle/add-edit-expense/add-edit-expense.page.ts

View workflow job for this annotation

GitHub Actions / Run linters 

Member getDeleteReportParams should be declared before all private instance method definitions
config: { header: string; body: string; ctaText: string; ctaLoadingText: string },
removeExpenseFromReport: boolean = false,
reportId?: string
Expand Down Expand Up @@ -4873,7 +4884,7 @@
};
}

async deleteExpense(reportId?: string): Promise<void> {

Check failure on line 4887 in src/app/fyle/add-edit-expense/add-edit-expense.page.ts

View workflow job for this annotation

GitHub Actions / Run linters 

Member deleteExpense should be declared before all private instance method definitions

Check failure on line 4887 in src/app/fyle/add-edit-expense/add-edit-expense.page.ts

View workflow job for this annotation

GitHub Actions / Run linters 

Member deleteExpense should be declared before all private instance method definitions
const removeExpenseFromReport = reportId && this.isRedirectedFromReport;
const header = removeExpenseFromReport ? 'Remove Expense' : 'Delete Expense';
const body = removeExpenseFromReport
Expand Down Expand Up @@ -4917,7 +4928,7 @@
}
}

async openCommentsModal(): Promise<void> {

Check failure on line 4931 in src/app/fyle/add-edit-expense/add-edit-expense.page.ts

View workflow job for this annotation

GitHub Actions / Run linters 

Member openCommentsModal should be declared before all private instance method definitions

Check failure on line 4931 in src/app/fyle/add-edit-expense/add-edit-expense.page.ts

View workflow job for this annotation

GitHub Actions / Run linters 

Member openCommentsModal should be declared before all private instance method definitions
const etxn = await this.etxn$.toPromise();

const modal = await this.modalController.create({
Expand Down
Loading