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
8 changes: 8 additions & 0 deletions src/app/core/mock-data/transaction.data.ts
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,14 @@ export const txnData: Transaction = deepFreeze({
skip_reimbursement: false,
});

export const txnDataCleaned: Partial<Transaction> = deepFreeze({
source: 'MOBILE_DASHCAM_BULK',
txn_dt: new Date('2023-02-08T17:00:00.000Z'),
currency: 'INR',
source_account_id: 'acc5APeygFjRd',
skip_reimbursement: false,
});

export const txnData2: Transaction = deepFreeze({
created_at: new Date('2023-02-08T06:47:48.414Z'),
updated_at: new Date('2023-02-08T06:47:48.414Z'),
Expand Down
8 changes: 7 additions & 1 deletion src/app/core/services/transaction.service.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ import {
txnData,
txnData2,
txnData4,
txnDataCleaned,
txnDataPayload,
txnList,
upsertTxnParam,
Expand Down Expand Up @@ -1100,9 +1101,14 @@ 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] });
expect(expensesService.createFromFile).toHaveBeenCalledOnceWith(mockFileObject[0].id, 'MOBILE_DASHCAM_BULK');
expect(transactionService.upsert).toHaveBeenCalledOnceWith({
...txnDataCleaned,
id: expenseData.id,
});
Comment on lines +1104 to +1111
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 enhancement, but let's make it even more powerful!

The test case looks good with the new assertions, but in true Rajinikanth style, let's make it bulletproof! Consider adding error case scenarios.

Add an error test case to verify the error handling:

it('should handle errors when creating transaction with files', (done) => {
  const mockFileObject = cloneDeep(fileObjectData1);
  const error = new Error('File creation failed');
  
  expensesService.createFromFile.and.returnValue(throwError(() => error));
  
  transactionService.createTxnWithFiles({ ...txnData }, of(mockFileObject)).subscribe({
    error: (err) => {
      expect(err).toBe(error);
      expect(expensesService.createFromFile).toHaveBeenCalledOnceWith(mockFileObject[0].id, 'MOBILE_DASHCAM_BULK');
      expect(transactionService.upsert).not.toHaveBeenCalled();
      done();
    }
  });
});

done();
});
});
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 @@ export class TransactionService {
): 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);
const isReceiptUpload = txn.hasOwnProperty('source') && Object.keys(txn).length === 1;
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Mind it! Let's make this code bulletproof, thalaiva style!

Replace hasOwnProperty with the more secure Object.hasOwn(). This prevents potential security issues when the prototype chain is tampered with.

Here's the power-packed fix:

-const isReceiptUpload = txn.hasOwnProperty('source') && Object.keys(txn).length === 1;
+const isReceiptUpload = Object.hasOwn(txn, 'source') && Object.keys(txn).length === 1;
📝 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 isReceiptUpload = txn.hasOwnProperty('source') && Object.keys(txn).length === 1;
const isReceiptUpload = Object.hasOwn(txn, 'source') && Object.keys(txn).length === 1;
🧰 Tools
🪛 Biome (1.9.4)

[error] 199-199: Do not access Object.prototype method 'hasOwnProperty' from target object.

It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.

(lint/suspicious/noPrototypeBuiltins)

const isAmountPresent = !!txn.amount;
if ((isReceiptUpload || !isAmountPresent) && fileIds.length > 0) {
return this.expensesService.createFromFile(fileIds[0], txn.source).pipe(
switchMap((result) => {
// capture receipt flow: patching the expense in case of amount not present
if (!isReceiptUpload && !isAmountPresent) {
txn.id = result.data[0].id;
return this.upsert(this.cleanupExpensePayload(txn));
} else {
return of(this.transformExpense(result.data[0]).tx);
}
})
);
} else {
const fileIds = fileObjs.map((fileObj) => fileObj.id);
txn.file_ids = fileIds;
return this.upsert(txn);
}
})
Expand Down Expand Up @@ -919,4 +924,15 @@ export class TransactionService {

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] as Transaction[keyof Transaction];
}
}
return newTxn;
}
Comment on lines +928 to +938
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Wait a minute! Let's tighten up this method with proper types.

The cleanupExpensePayload method lacks a return type annotation, and the implementation can be more type-safe and readable.

Here's how you can enhance it:

-private cleanupExpensePayload(txn: Partial<Transaction>) {
-  const newTxn = {};
+private cleanupExpensePayload(txn: Partial<Transaction>): Partial<Transaction> {
+  const newTxn: Partial<Transaction> = {};
   for (const key in txn) {
-    if (txn[key] !== null && txn[key] !== undefined) {
+    if (txn[key] != null) {
       newTxn[key] = txn[key] as Transaction[keyof Transaction];
     }
   }
   return newTxn;
 }

Alternatively, you can use Object.entries for better readability:

private cleanupExpensePayload(txn: Partial<Transaction>): Partial<Transaction> {
  return Object.fromEntries(
    Object.entries(txn).filter(([_, value]) => value != null)
  ) as Partial<Transaction>;
}

Comment on lines +929 to +938
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 it! Let's add some punch to this code with proper types!

The method needs some power-packed improvements:

  1. Add proper return type annotation
  2. Use type-safe implementation
  3. Consider using Object.entries for better readability

Here's the superstar way to do it:

- private cleanupExpensePayload(txn: Partial<Transaction>) {
-   const newTxn = {};
+ private cleanupExpensePayload(txn: Partial<Transaction>): Partial<Transaction> {
+   const newTxn: Partial<Transaction> = {};
    for (const key in txn) {
-     if (txn[key] !== null && txn[key] !== undefined) {
+     if (txn[key] != null) {
        newTxn[key] = txn[key] as Transaction[keyof Transaction];
      }
    }
    return newTxn;
  }

Alternative implementation using Object.entries:

private cleanupExpensePayload(txn: Partial<Transaction>): Partial<Transaction> {
  return Object.fromEntries(
    Object.entries(txn).filter(([_, value]) => value != null)
  ) as Partial<Transaction>;
}

}
4 changes: 0 additions & 4 deletions src/app/core/services/transactions-outbox.service.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -178,13 +178,11 @@ describe('TransactionsOutboxService', () => {
transactionsOutboxService.addEntryAndSync(
txnData2,
[{ url: '2023-02-08/orNVthTo2Zyo/receipts/fi6PQ6z4w6ET.000.jpeg', type: 'image/jpeg' }],
null,
null
);
expect(transactionsOutboxService.addEntry).toHaveBeenCalledOnceWith(
txnData2,
[{ url: '2023-02-08/orNVthTo2Zyo/receipts/fi6PQ6z4w6ET.000.jpeg', type: 'image/jpeg' }],
null,
null
);
expect(transactionsOutboxService.syncEntry).toHaveBeenCalledOnceWith(outboxQueueData1[0]);
Expand All @@ -195,13 +193,11 @@ describe('TransactionsOutboxService', () => {
transactionsOutboxService.addEntryAndSync(
txnData2,
[{ url: '2023-02-08/orNVthTo2Zyo/receipts/fi6PQ6z4w6ET.000.jpeg', type: 'image/jpeg' }],
null,
null
);
expect(transactionsOutboxService.addEntry).toHaveBeenCalledOnceWith(
txnData2,
[{ url: '2023-02-08/orNVthTo2Zyo/receipts/fi6PQ6z4w6ET.000.jpeg', type: 'image/jpeg' }],
null,
null
);
expect(transactionsOutboxService.syncEntry).toHaveBeenCalledOnceWith(outboxQueueData1[0]);
Expand Down
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: 3 additions & 9 deletions src/app/fyle/add-edit-expense/add-edit-expense-4.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -481,8 +481,7 @@ export function TestCases4(getTestBed) {
expect(transactionOutboxService.addEntry).toHaveBeenCalledOnceWith(
mockEtxn.tx,
[{ url: '2023-02-08/orNVthTo2Zyo/receipts/fi6PQ6z4w6ET.000.pdf', type: 'pdf' }],
[],
'rprAfNrce73O'
[]
);
done();
});
Expand Down Expand Up @@ -523,12 +522,7 @@ export function TestCases4(getTestBed) {
);
expect(authService.getEou).toHaveBeenCalledOnceWith();
expect(component.trackCreateExpense).toHaveBeenCalledOnceWith(expectedUnflattendedTxnData3, false);
expect(transactionOutboxService.addEntry).toHaveBeenCalledOnceWith(
outboxQueueData1[0].transaction,
[],
[],
undefined
);
expect(transactionOutboxService.addEntry).toHaveBeenCalledOnceWith(outboxQueueData1[0].transaction, [], []);
done();
});
});
Expand Down Expand Up @@ -574,7 +568,7 @@ export function TestCases4(getTestBed) {
);
expect(authService.getEou).toHaveBeenCalledOnceWith();
expect(component.trackCreateExpense).toHaveBeenCalledOnceWith(expectedUnflattendedTxnData4, false);
expect(transactionOutboxService.addEntry).toHaveBeenCalledOnceWith(mockEtxn.tx, [], ['continue'], undefined);
expect(transactionOutboxService.addEntry).toHaveBeenCalledOnceWith(mockEtxn.tx, [], ['continue']);
done();
});
});
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
41 changes: 25 additions & 16 deletions src/app/fyle/add-edit-mileage/add-edit-mileage-3.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,7 @@ export function TestCases3(getTestBed) {
spyOn(component, 'getCustomFields').and.returnValue(of(txnCustomPropertiesData4));
spyOn(component, 'getCalculatedDistance').and.returnValue(of('10'));
component.isConnected$ = of(true);
spyOn(component, 'generateEtxnFromFg').and.returnValue(of(unflattenedTxnData));
spyOn(component, 'generateEtxnFromFg').and.returnValue(of(cloneDeep(unflattenedTxnData)));
spyOn(component, 'checkPolicyViolation').and.returnValue(of(expensePolicyDataWoData));
policyService.getCriticalPolicyRules.and.returnValue([]);
policyService.getPolicyRules.and.returnValue([]);
Expand All @@ -256,11 +256,18 @@ export function TestCases3(getTestBed) {
spyOn(component, 'getFormValues').and.returnValue({
report: expectedReportsPaginated[0],
});
const expectedEtxnData = {
...unflattenedTxnData,
tx: {
...unflattenedTxnData.tx,
report_id: expectedReportsPaginated[0].id,
},
};
transactionOutboxService.addEntryAndSync.and.resolveTo(outboxQueueData1[0]);
fixture.detectChanges();

component.addExpense('SAVE_MILEAGE').subscribe((res) => {
expect(res).toEqual(unflattenedTxnData);
expect(res).toEqual(expectedEtxnData);
expect(component.getCustomFields).toHaveBeenCalledTimes(1);
expect(component.getCalculatedDistance).toHaveBeenCalledTimes(1);
expect(component.generateEtxnFromFg).toHaveBeenCalledOnceWith(
Expand All @@ -274,10 +281,9 @@ export function TestCases3(getTestBed) {
expect(component.trackCreateExpense).toHaveBeenCalledTimes(1);
expect(component.getFormValues).toHaveBeenCalledTimes(1);
expect(transactionOutboxService.addEntryAndSync).toHaveBeenCalledOnceWith(
unflattenedTxnData.tx,
expectedEtxnData.tx,
unflattenedTxnData.dataUrls as any,
[],
expectedReportsPaginated[0].id
[]
);
done();
});
Expand Down Expand Up @@ -318,8 +324,7 @@ export function TestCases3(getTestBed) {
expect(transactionOutboxService.addEntryAndSync).toHaveBeenCalledOnceWith(
unflattenedMileageDataWithPolicyAmount.tx,
unflattenedTxnData.dataUrls as any,
[],
undefined
[]
);
done();
Comment on lines +327 to 329
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Don't disturb! The empty array parameter needs documentation.

The empty array parameter [] in addEntryAndSync calls appears to be for comments, but its purpose isn't immediately clear.

Add a comment to clarify the purpose of the empty array:

 expect(transactionOutboxService.addEntryAndSync).toHaveBeenCalledOnceWith(
   unflattenedMileageDataWithPolicyAmount.tx,
   unflattenedTxnData.dataUrls as any,
-  []
+  [] // comments array - empty when no policy violations
 );

Also applies to: 429-431

});
Expand Down Expand Up @@ -369,8 +374,7 @@ export function TestCases3(getTestBed) {
expect(transactionOutboxService.addEntryAndSync).toHaveBeenCalledOnceWith(
unflattenedTxnData.tx,
unflattenedTxnData.dataUrls as any,
[],
undefined
[]
);
done();
});
Expand Down Expand Up @@ -422,8 +426,7 @@ export function TestCases3(getTestBed) {
expect(transactionOutboxService.addEntryAndSync).toHaveBeenCalledOnceWith(
unflattendedTxnWithPolicyAmount.tx,
unflattendedTxnWithPolicyAmount.dataUrls as any,
['A comment'],
undefined
['A comment']
);
done();
});
Expand All @@ -433,18 +436,25 @@ export function TestCases3(getTestBed) {
component.isConnected$ = of(false);
spyOn(component, 'getCustomFields').and.returnValue(of(txnCustomPropertiesData4));
spyOn(component, 'getCalculatedDistance').and.returnValue(of('10'));
spyOn(component, 'generateEtxnFromFg').and.returnValue(of(unflattenedTxnData));
spyOn(component, 'generateEtxnFromFg').and.returnValue(of(cloneDeep(unflattenedTxnData)));
spyOn(component, 'checkPolicyViolation').and.returnValue(of(expensePolicyDataWoData));
spyOn(component, 'trackCreateExpense');
authService.getEou.and.resolveTo(apiEouRes);
spyOn(component, 'getFormValues').and.returnValue({
report: expectedReportsPaginated[0],
});
const expectedEtxnData = {
...unflattenedTxnData,
tx: {
...unflattenedTxnData.tx,
report_id: expectedReportsPaginated[0].id,
},
};
Comment on lines +439 to +452
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Kabali da! The offline mode test case needs better structure.

The offline mode test case duplicates the expectedEtxnData object construction. This could be moved to a shared setup.

Extract the expected data setup:

+let expectedOfflineEtxnData;
+beforeEach(() => {
+  expectedOfflineEtxnData = {
+    ...unflattenedTxnData,
+    tx: {
+      ...unflattenedTxnData.tx,
+      report_id: expectedReportsPaginated[0].id,
+    },
+  };
+});

 it('should add expense in offline mode', (done) => {
   component.isConnected$ = of(false);
   spyOn(component, 'getCustomFields').and.returnValue(of(txnCustomPropertiesData4));
   spyOn(component, 'getCalculatedDistance').and.returnValue(of('10'));
-  spyOn(component, 'generateEtxnFromFg').and.returnValue(of(cloneDeep(unflattenedTxnData)));
+  spyOn(component, 'generateEtxnFromFg').and.returnValue(of(expectedOfflineEtxnData));

Also applies to: 457-471

transactionOutboxService.addEntryAndSync.and.resolveTo(outboxQueueData1[0]);
fixture.detectChanges();

component.addExpense('SAVE_MILEAGE').subscribe((res) => {
expect(res).toEqual(unflattenedTxnData);
expect(res).toEqual(expectedEtxnData);
expect(component.getCustomFields).toHaveBeenCalledTimes(1);
expect(component.getCalculatedDistance).toHaveBeenCalledTimes(1);
expect(component.generateEtxnFromFg).toHaveBeenCalledOnceWith(
Expand All @@ -456,10 +466,9 @@ export function TestCases3(getTestBed) {
expect(component.trackCreateExpense).toHaveBeenCalledTimes(1);
expect(component.getFormValues).toHaveBeenCalledTimes(1);
expect(transactionOutboxService.addEntryAndSync).toHaveBeenCalledOnceWith(
unflattenedTxnData.tx,
expectedEtxnData.tx,
unflattenedTxnData.dataUrls as any,
[],
expectedReportsPaginated[0].id
[]
);
done();
});
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
Loading
Loading