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

fix: Allow save expense with invalid fields after capture receipt #3308

Merged
merged 12 commits into from
Dec 9, 2024
1 change: 1 addition & 0 deletions src/app/core/services/transaction.service.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1100,6 +1100,7 @@ describe('TransactionService', () => {
const mockFileObject = cloneDeep(fileObjectData1);

spyOn(transactionService, 'upsert').and.returnValue(of(txnData2));
expensesService.createFromFile.and.returnValue(of({ data: [expenseData] }));
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Mind-blowing test case, but let's make it more robust!

The test case looks good, but in true Rajinikanth style, let's make it even more powerful! Consider adding assertions to verify that expensesService.createFromFile was called with the correct parameters.

Add this assertion after the existing expect statements:

 transactionService.createTxnWithFiles({ ...txnData }, of(mockFileObject)).subscribe((res) => {
   expect(res).toEqual(txnData2);
   expect(transactionService.upsert).toHaveBeenCalledOnceWith({ ...txnData, file_ids: [fileObjectData1[0].id] });
+  expect(expensesService.createFromFile).toHaveBeenCalledOnceWith(mockFileObject[0].id, txnData.source);
   done();
 });

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

transactionService.createTxnWithFiles({ ...txnData }, of(mockFileObject)).subscribe((res) => {
expect(res).toEqual(txnData2);
expect(transactionService.upsert).toHaveBeenCalledOnceWith({ ...txnData, file_ids: [fileObjectData1[0].id] });
Expand Down
36 changes: 26 additions & 10 deletions src/app/core/services/transaction.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -195,17 +195,22 @@
): Observable<Partial<Transaction>> {
return fileUploads$.pipe(
switchMap((fileObjs) => {
// txn contains only source key when capturing receipt
if (txn.hasOwnProperty('source') && Object.keys(txn).length === 1) {
const fileIds = fileObjs.map((fileObj) => fileObj.id);
if (fileIds.length > 0) {
return this.expensesService
.createFromFile(fileIds[0], txn.source)
.pipe(map((result) => this.transformExpense(result.data[0]).tx));
}
const fileIds = fileObjs.map((fileObj) => fileObj.id);
if (fileIds.length > 0) {
return this.expensesService.createFromFile(fileIds[0], txn.source).pipe(
switchMap((result) => {
// txn contains only source key when capturing receipt in bulk mode
if (txn.hasOwnProperty('source') && Object.keys(txn).length === 1) {
return of(this.transformExpense(result.data[0]).tx);
} else {
// capture receipt flow: patching the expense with any of the form values.
txn.file_ids = fileIds;
txn.id = result.data[0].id;
return this.upsert(this.cleanupExpensePayload(txn));
}
})
);
} else {
const fileIds = fileObjs.map((fileObj) => fileObj.id);
txn.file_ids = fileIds;
return this.upsert(txn);
}
})
Expand Down Expand Up @@ -919,4 +924,15 @@

return typeOrFilter;
}

// to be used only when updating created expense with form values during capture recept flow
private cleanupExpensePayload(txn: Partial<Transaction>): Partial<Transaction> {
const newTxn: Partial<Transaction> = {};
for (const key in txn) {
if (txn[key] !== null && txn[key] !== undefined) {
newTxn[key] = txn[key];

Check failure on line 933 in src/app/core/services/transaction.service.ts

View workflow job for this annotation

GitHub Actions / Run linters

Unsafe assignment of an `any` value

Check failure on line 933 in src/app/core/services/transaction.service.ts

View workflow job for this annotation

GitHub Actions / Run linters

Unsafe assignment of an `any` value
}
}
return newTxn;
}
}
9 changes: 3 additions & 6 deletions src/app/core/services/transactions-outbox.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -141,14 +141,12 @@ export class TransactionsOutboxService {
addEntry(
transaction: Partial<Transaction>,
dataUrls: { url: string; type: string }[],
comments?: string[],
reportId?: string
comments?: string[]
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Mind this! Method signature updated but calls may be outdated.

You've removed the reportId parameter from addEntry. Ensure all calls to this method are updated accordingly.

): Promise<void> {
this.queue.push({
transaction,
dataUrls,
comments,
reportId,
});

return this.saveQueue();
Expand All @@ -159,10 +157,9 @@ export class TransactionsOutboxService {
addEntryAndSync(
transaction: Partial<Transaction>,
dataUrls: { url: string; type: string }[],
comments: string[],
reportId: string
comments: string[]
): Promise<OutboxQueue> {
this.addEntry(transaction, dataUrls, comments, reportId);
this.addEntry(transaction, dataUrls, comments);
return this.syncEntry(this.queue.pop());
}

Expand Down
12 changes: 4 additions & 8 deletions src/app/fyle/add-edit-expense/add-edit-expense.page.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4275,7 +4275,6 @@ export class AddEditExpensePage implements OnInit {
const customFields$ = this.getCustomFields();

this.trackAddExpense();

return this.generateEtxnFromFg(this.etxn$, customFields$).pipe(
switchMap((etxn) =>
this.isConnected$.pipe(
Expand Down Expand Up @@ -4361,13 +4360,12 @@ export class AddEditExpensePage implements OnInit {
etxn.tx.matchCCCId = this.selectedCCCTransaction.id;
}

let reportId: string;
const formValues = this.getFormValues();
if (
formValues.report &&
(etxn.tx.policy_amount === null || (etxn.tx.policy_amount && !(etxn.tx.policy_amount < 0.0001)))
) {
reportId = formValues.report.id;
etxn.tx.report_id = formValues.report.id;
}

etxn.dataUrls = etxn.dataUrls.map((data: FileObject) => {
Expand All @@ -4387,8 +4385,7 @@ export class AddEditExpensePage implements OnInit {
this.transactionOutboxService.addEntryAndSync(
etxn.tx,
etxn.dataUrls as { url: string; type: string }[],
comments,
reportId
comments
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Watch out! Incorrect parameter passed to addEntryAndSync.

The method addEntryAndSync might have an updated signature. Confirm that all required parameters are correctly passed.

Here's a suggested change:

- this.transactionOutboxService.addEntryAndSync(
-   etxn.tx,
-   etxn.dataUrls as { url: string; type: string }[],
-   comments
- );
+ this.transactionOutboxService.addEntryAndSync(
+   etxn.tx,
+   etxn.dataUrls as { url: string; type: string }[],
+   comments
+ );

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

)
);
} else {
Expand All @@ -4404,13 +4401,12 @@ export class AddEditExpensePage implements OnInit {
this.transactionOutboxService.addEntryAndSync(
etxn.tx,
etxn.dataUrls as { url: string; type: string }[],
comments,
reportId
comments
)
);
} else {
this.transactionOutboxService
.addEntry(etxn.tx, etxn.dataUrls as { url: string; type: string }[], comments, reportId)
.addEntry(etxn.tx, etxn.dataUrls as { url: string; type: string }[], comments)
.then(noop);

return of(null);
Expand Down
6 changes: 2 additions & 4 deletions src/app/fyle/add-edit-mileage/add-edit-mileage.page.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2910,19 +2910,17 @@ export class AddEditMileagePage implements OnInit {

const reportValue = this.getFormValues();

let reportId: string;
if (
reportValue?.report &&
(etxn.tx.policy_amount === null || (etxn.tx.policy_amount && !(etxn.tx.policy_amount < 0.0001)))
) {
reportId = reportValue.report?.id;
etxn.tx.report_id = reportValue.report?.id;
}
return of(
this.transactionsOutboxService.addEntryAndSync(
etxn.tx,
etxn.dataUrls as { url: string; type: string }[],
comments,
reportId
comments
)
).pipe(
switchMap((txnData: Promise<unknown>) => from(txnData)),
Expand Down
6 changes: 2 additions & 4 deletions src/app/fyle/add-edit-per-diem/add-edit-per-diem.page.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1970,20 +1970,18 @@ export class AddEditPerDiemPage implements OnInit {
comments.push(comment);
}

let reportId: string;
const formValue = this.getFormValues();
if (
formValue.report &&
(etxn.tx.policy_amount === null || (etxn.tx.policy_amount && !(etxn.tx.policy_amount < 0.0001)))
) {
reportId = formValue.report.id;
etxn.tx.report_id = formValue.report.id;
}
return of(
this.transactionsOutboxService.addEntryAndSync(
etxn.tx,
etxn.dataUrls as { url: string; type: string }[],
comments,
reportId
comments
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Hey there! Let's make sure we're handling formValue.report properly.

We're assigning formValue.report.id to etxn.tx.report_id without checking if formValue.report exists. It's always safer to check before we act.

Here's a suggestion:

1978,1980
- if (formValue.report && (etxn.tx.policy_amount === null || (etxn.tx.policy_amount && !(etxn.tx.policy_amount < 0.0001)))) {
-   etxn.tx.report_id = formValue.report.id;
- }
+ if (
+   formValue.report &&
+   formValue.report.id &&
+   (etxn.tx.policy_amount === null || (etxn.tx.policy_amount && !(etxn.tx.policy_amount < 0.0001)))
+ ) {
+   etxn.tx.report_id = formValue.report.id;
+ }
📝 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
etxn.tx.report_id = formValue.report.id;
}
return of(
this.transactionsOutboxService.addEntryAndSync(
etxn.tx,
etxn.dataUrls as { url: string; type: string }[],
comments,
reportId
comments
if (
formValue.report &&
formValue.report.id &&
(etxn.tx.policy_amount === null || (etxn.tx.policy_amount && !(etxn.tx.policy_amount < 0.0001)))
) {
etxn.tx.report_id = formValue.report.id;
}
return of(
this.transactionsOutboxService.addEntryAndSync(
etxn.tx,
etxn.dataUrls as { url: string; type: string }[],
comments

)
).pipe(switchMap((txnData) => from(txnData)));
})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ export class CaptureReceiptComponent implements OnInit, OnDestroy, AfterViewInit
url: base64ImagesWithSource.base64Image,
},
];
return this.transactionsOutboxService.addEntry(transaction, attachmentUrls, null, null);
return this.transactionsOutboxService.addEntry(transaction, attachmentUrls, null);
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Wait a second! Missing argument in method call.

In this.transactionsOutboxService.addEntry, you've reduced parameters. Ensure this aligns with the updated method signature.

Here's how you might fix it:

- return this.transactionsOutboxService.addEntry(transaction, attachmentUrls, null);
+ return this.transactionsOutboxService.addEntry(transaction, attachmentUrls);

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

})
);
}
Expand Down
Loading