fix: Allow save expense with invalid fields after capture receipt #3308
Conversation
WalkthroughThe pull request modifies the Changes
Possibly related PRs
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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
src/app/core/services/transaction.service.ts(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/app/core/services/transaction.service.ts
[error] 203-203: 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)
🪛 eslint
src/app/core/services/transaction.service.ts
[error] 928-928: Missing return type on function.
(@typescript-eslint/explicit-function-return-type)
🪛 GitHub Check: Run linters
src/app/core/services/transaction.service.ts
[failure] 928-928:
Missing return type on function
[failure] 932-932:
Unsafe assignment of an any value
| private cleanupExpenseCreatePayload(txn: Partial<Transaction>) { | ||
| const newTxn = {}; | ||
| for (const key in txn) { | ||
| if (txn[key] !== null && txn[key] !== undefined) { | ||
| newTxn[key] = txn[key]; | ||
| } | ||
| } | ||
| return newTxn; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mind it! Let's add some punch to this code with proper types!
The method needs some power-packed improvements:
- Add proper return type annotation
- Use type-safe implementation
- Consider using Object.entries for better readability
Here's the superstar way to do it:
- private cleanupExpenseCreatePayload(txn: Partial<Transaction>) {
- const newTxn = {};
+ private cleanupExpenseCreatePayload(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];
}
}
return newTxn;
}Alternative implementation using Object.entries:
private cleanupExpenseCreatePayload(txn: Partial<Transaction>): Partial<Transaction> {
return Object.fromEntries(
Object.entries(txn).filter(([_, value]) => value != null)
) as Partial<Transaction>;
}🧰 Tools
🪛 eslint
[error] 928-928: Missing return type on function.
(@typescript-eslint/explicit-function-return-type)
🪛 GitHub Check: Run linters
[failure] 928-928:
Missing return type on function
[failure] 932-932:
Unsafe assignment of an any value
| 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 | ||
| if (txn.hasOwnProperty('source') && Object.keys(txn).length === 1) { | ||
| return of(this.transformExpense(result.data[0]).tx); | ||
| } else { | ||
| const fileIds = fileObjs.map((fileObj) => fileObj.id); | ||
| txn.file_ids = fileIds; | ||
| txn.id = result.data[0].id; | ||
| return this.upsert(this.cleanupExpenseCreatePayload(txn)); | ||
| } | ||
| }) | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Listen up, partner! Let's make this code more stylish than my signature walk!
The code needs some style upgrades in the following areas:
- Replace
hasOwnPropertywithObject.hasOwn()for better safety - Add proper type annotations for better type safety
Here's how to make it super:
return fileUploads$.pipe(
switchMap((fileObjs) => {
const fileIds = fileObjs.map((fileObj) => fileObj.id);
if (fileIds.length > 0) {
return this.expensesService.createFromFile(fileIds[0], txn.source).pipe(
switchMap((result) => {
- if (txn.hasOwnProperty('source') && Object.keys(txn).length === 1) {
+ if (Object.hasOwn(txn, 'source') && Object.keys(txn).length === 1) {
return of(this.transformExpense(result.data[0]).tx);
} else {
const fileIds = fileObjs.map((fileObj) => fileObj.id);
txn.file_ids = fileIds;
txn.id = result.data[0].id;
return this.upsert(this.cleanupExpenseCreatePayload(txn));
}
})
);📝 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.
| 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 | |
| if (txn.hasOwnProperty('source') && Object.keys(txn).length === 1) { | |
| return of(this.transformExpense(result.data[0]).tx); | |
| } else { | |
| const fileIds = fileObjs.map((fileObj) => fileObj.id); | |
| txn.file_ids = fileIds; | |
| txn.id = result.data[0].id; | |
| return this.upsert(this.cleanupExpenseCreatePayload(txn)); | |
| } | |
| }) | |
| ); | |
| 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 | |
| if (Object.hasOwn(txn, 'source') && Object.keys(txn).length === 1) { | |
| return of(this.transformExpense(result.data[0]).tx); | |
| } else { | |
| const fileIds = fileObjs.map((fileObj) => fileObj.id); | |
| txn.file_ids = fileIds; | |
| txn.id = result.data[0].id; | |
| return this.upsert(this.cleanupExpenseCreatePayload(txn)); | |
| } | |
| }) | |
| ); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 203-203: 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)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
src/app/core/services/transaction.service.ts(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/app/core/services/transaction.service.ts
[error] 203-203: 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)
🪛 eslint
src/app/core/services/transaction.service.ts
[error] 929-929: Missing return type on function.
(@typescript-eslint/explicit-function-return-type)
🪛 GitHub Check: Run linters
src/app/core/services/transaction.service.ts
[failure] 929-929:
Missing return type on function
[failure] 933-933:
Unsafe assignment of an any value
🔇 Additional comments (2)
src/app/core/services/transaction.service.ts (2)
Line range hint 198-937: Mind it! The changes look solid as a rock!
The implementation successfully achieves the goal of allowing expenses to be saved with invalid fields after quick fyle. The new cleanupExpensePayload method effectively filters out invalid properties, making the code more robust.
🧰 Tools
🪛 Biome (1.9.4)
[error] 203-203: 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)
198-212: 🛠️ Refactor suggestion
Mind it! Let's make this code more powerful than a punch dialogue!
The code needs some style upgrades, macha! Here's how to make it super:
- Replace
hasOwnPropertywithObject.hasOwn()for better safety - Add proper type annotations for better type safety
return fileUploads$.pipe(
switchMap((fileObjs) => {
const fileIds = fileObjs.map((fileObj) => fileObj.id);
if (fileIds.length > 0) {
return this.expensesService.createFromFile(fileIds[0], txn.source).pipe(
switchMap((result) => {
- if (txn.hasOwnProperty('source') && Object.keys(txn).length === 1) {
+ if (Object.hasOwn(txn, 'source') && Object.keys(txn).length === 1) {
return of(this.transformExpense(result.data[0]).tx);
} else {
const fileIds = fileObjs.map((fileObj) => fileObj.id);
txn.file_ids = fileIds;
txn.id = result.data[0].id;
return this.upsert(this.cleanupExpensePayload(txn));
}
})
);Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 203-203: 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)
| // to be used only when updating created expense with form values during capture recept flow | ||
| private cleanupExpensePayload(txn: Partial<Transaction>) { | ||
| const newTxn = {}; | ||
| for (const key in txn) { | ||
| if (txn[key] !== null && txn[key] !== undefined) { | ||
| newTxn[key] = txn[key]; | ||
| } | ||
| } | ||
| return newTxn; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Style ah? Let's add some punch to this code with proper types!
The method needs some power-packed improvements, thalaiva:
- Add proper return type annotation
- Use type-safe implementation
- Consider using Object.entries for better readability
- 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];
}
}
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>;
}🧰 Tools
🪛 eslint
[error] 929-929: Missing return type on function.
(@typescript-eslint/explicit-function-return-type)
🪛 GitHub Check: Run linters
[failure] 929-929:
Missing return type on function
[failure] 933-933:
Unsafe assignment of an any value
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range comments (3)
src/app/fyle/add-edit-per-diem/add-edit-per-diem.page.ts (3)
Line range hint
984-989: Listen up! IncheckIfInvalidPaymentMode, we're subscribing within a subscription.This might lead to unexpected behaviors or memory leaks. Let's refactor it by using RxJS operators instead of nested subscriptions.
Consider refactoring like this:
984,989 - that - .checkIfInvalidPaymentMode() - .pipe(take(1)) - .subscribe((invalidPaymentMode) => { - if (that.fg.valid && !invalidPaymentMode) { + this.checkIfInvalidPaymentMode().pipe( + take(1), + switchMap((invalidPaymentMode) => { + if (this.fg.valid && !invalidPaymentMode) {And ensure to handle the rest of the logic within the
switchMap.
Line range hint
1279-1284: Hold on! Let's be cautious withdependentFields$initialization.We're calling
customInputsService.getAll(true)multiple times. It's better to share this observable.You might want to use
shareReplay(1)to avoid unnecessary calls:1279,1281 - const customExpenseFields$ = this.customInputsService.getAll(true).pipe(shareReplay(1)); - - this.dependentFields$ = customExpenseFields$.pipe( + this.dependentFields$ = this.customInputs$.pipe(Ensure that
this.customInputs$is initialized appropriately.
Line range hint
1490-1495: Heads up! The error handling inaddExpensecould be improved.We're using multiple nested
catchError. Let's simplify it for better readability.Consider refactoring the
catchErrorblock to handle different error types more cleanly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (7)
src/app/core/services/transaction.service.spec.ts(1 hunks)src/app/core/services/transaction.service.ts(2 hunks)src/app/core/services/transactions-outbox.service.ts(2 hunks)src/app/fyle/add-edit-expense/add-edit-expense.page.ts(3 hunks)src/app/fyle/add-edit-mileage/add-edit-mileage.page.ts(1 hunks)src/app/fyle/add-edit-per-diem/add-edit-per-diem.page.ts(1 hunks)src/app/shared/components/capture-receipt/capture-receipt.component.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/app/core/services/transaction.service.ts
[error] 203-203: 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)
🪛 GitHub Check: Run linters
src/app/core/services/transaction.service.ts
[failure] 933-933:
Unsafe assignment of an any value
🔇 Additional comments (7)
src/app/fyle/add-edit-per-diem/add-edit-per-diem.page.ts (1)
1984-1984: 🧹 Nitpick (assertive)
Watch out! It seems there's a misaligned closing brace here.
Alignment and indentation keep our code neat and tidy. Let's adjust it for better readability.
Here's the corrected indentation:
1984
- ).pipe(switchMap((txnData) => from(txnData)));
+ ).pipe(switchMap((txnData) => from(txnData)));Likely invalid or redundant comment.
src/app/core/services/transaction.service.ts (2)
203-203: Boss, avoid using 'hasOwnProperty' directly on objects
Using hasOwnProperty directly can lead to unexpected issues. Better to use Object.hasOwn() for safety.
Apply this diff to fix the issue:
- if (txn.hasOwnProperty('source') && Object.keys(txn).length === 1) {
+ if (Object.hasOwn(txn, 'source') && Object.keys(txn).length === 1) {🧰 Tools
🪛 Biome (1.9.4)
[error] 203-203: 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)
929-937: Let's make 'cleanupExpensePayload' more powerful
Add return type annotations and consider using Object.entries for cleaner code and better type safety.
Apply this diff to improve the method:
- 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];
}
}
return newTxn;
}Alternatively, you could use Object.entries:
private cleanupExpensePayload(txn: Partial<Transaction>): Partial<Transaction> {
return Object.fromEntries(
Object.entries(txn).filter(([_, value]) => value != null)
) as Partial<Transaction>;
}🧰 Tools
🪛 GitHub Check: Run linters
[failure] 933-933:
Unsafe assignment of an any value
src/app/fyle/add-edit-mileage/add-edit-mileage.page.ts (2)
2917-2917: Heads up! Assigning report ID directly may cause issues.
At line 2917, you're setting etxn.tx.report_id = reportValue.report?.id;. Make sure reportValue.report is not null or undefined to avoid unexpected behaviors.
2923-2923:
Wait a moment! Missing parameter in method call.
In this.transactionsOutboxService.addEntryAndSync, it seems you're missing a required parameter. Double-check the method definition to ensure all necessary arguments are provided.
Apply this diff to fix the issue:
- this.transactionsOutboxService.addEntryAndSync(
- etxn.tx,
- etxn.dataUrls as { url: string; type: string }[],
- comments
- );
+ this.transactionsOutboxService.addEntryAndSync(
+ etxn.tx,
+ etxn.dataUrls as { url: string; type: string }[],
+ comments
+ );Likely invalid or redundant comment.
src/app/fyle/add-edit-expense/add-edit-expense.page.ts (1)
4368-4368: Hold on! Direct assignment of report ID might cause issues.
At line 4368, assigning etxn.tx.report_id = formValues.report.id; could override existing report associations. Ensure this is intended and formValues.report is defined.
src/app/core/services/transactions-outbox.service.ts (1)
160-162: 🛠️ Refactor suggestion
Heads up! Update method calls to match new signature.
The addEntryAndSync method no longer requires reportId. Check all invocations to prevent runtime errors.
Update method calls like so:
- return this.addEntryAndSync(transaction, dataUrls, comments, reportId);
+ return this.addEntryAndSync(transaction, dataUrls, comments);Likely invalid or redundant comment.
| etxn.tx.report_id = formValue.report.id; | ||
| } | ||
| return of( | ||
| this.transactionsOutboxService.addEntryAndSync( | ||
| etxn.tx, | ||
| etxn.dataUrls as { url: string; type: string }[], | ||
| comments, | ||
| reportId | ||
| comments |
There was a problem hiding this comment.
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.
| 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 |
| @@ -1100,6 +1100,7 @@ describe('TransactionService', () => { | |||
| const mockFileObject = cloneDeep(fileObjectData1); | |||
|
|
|||
| spyOn(transactionService, 'upsert').and.returnValue(of(txnData2)); | |||
| expensesService.createFromFile.and.returnValue(of({ data: [expenseData] })); | |||
There was a problem hiding this comment.
🧹 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.
| @@ -4387,8 +4385,7 @@ export class AddEditExpensePage implements OnInit { | |||
| this.transactionOutboxService.addEntryAndSync( | |||
| etxn.tx, | |||
| etxn.dataUrls as { url: string; type: string }[], | |||
| comments, | |||
| reportId | |||
| comments | |||
There was a problem hiding this comment.
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.
| @@ -141,14 +141,12 @@ export class TransactionsOutboxService { | |||
| addEntry( | |||
| transaction: Partial<Transaction>, | |||
| dataUrls: { url: string; type: string }[], | |||
| comments?: string[], | |||
| reportId?: string | |||
| comments?: string[] | |||
There was a problem hiding this comment.
🛠️ 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.
| @@ -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); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (7)
src/app/core/mock-data/transaction.data.ts(1 hunks)src/app/core/services/transaction.service.spec.ts(2 hunks)src/app/core/services/transaction.service.ts(2 hunks)src/app/core/services/transactions-outbox.service.spec.ts(0 hunks)src/app/fyle/add-edit-expense/add-edit-expense-4.spec.ts(3 hunks)src/app/fyle/add-edit-mileage/add-edit-mileage-3.spec.ts(5 hunks)src/app/fyle/add-edit-per-diem/add-edit-per-diem-3.page.spec.ts(8 hunks)
💤 Files with no reviewable changes (1)
- src/app/core/services/transactions-outbox.service.spec.ts
🧰 Additional context used
🪛 Biome (1.9.4)
src/app/core/services/transaction.service.ts
[error] 203-203: 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)
🔇 Additional comments (13)
src/app/fyle/add-edit-per-diem/add-edit-per-diem-3.page.spec.ts (2)
675-675: Mind it! The test cases are properly handling the no-comment scenarios, macha!
The empty array for the comments parameter in addEntryAndSync correctly reflects that no policy violation comments are needed when there are no violations. This maintains data consistency, thalaiva!
Also applies to: 885-885, 915-915, 955-955
717-717: Kabali da! The policy violation comments are being passed correctly!
The test cases are properly verifying that policy violation comments (['comment']) are passed to addEntryAndSync when violations occur. This ensures that important policy context is preserved during sync, what a style!
Also applies to: 762-762, 805-805, 844-844
src/app/fyle/add-edit-mileage/add-edit-mileage-3.spec.ts (6)
279-279: Mind it! The change to pass empty array for comments is spot on!
The modification to pass [] instead of undefined for the comments parameter in addEntryAndSync makes the code more robust and predictable.
320-320: Thalaiva approves this change!
Consistently using empty array for comments parameter maintains uniformity across test cases.
370-370: Another blockbuster change, passing empty array for critical policy violation test!
The modification aligns with other test cases and improves consistency.
422-422: What a powerful scene! Comments array with actual comment for policy violation.
The test correctly verifies that policy violation comments are properly passed through.
457-457: Even in offline mode, empty array is the style!
Maintaining consistency by using empty array for comments in offline mode test case as well.
Line range hint 1-900: Let me tell you one thing - these test cases need more verification!
While the changes look good, we should verify if there are any edge cases we're not testing.
Consider adding test cases for:
- Empty string in comments array
- Multiple comments in array
- Null values in comments array
src/app/fyle/add-edit-expense/add-edit-expense-4.spec.ts (1)
Line range hint 44-44: Superstar addition!
The introduction of txnDataCleaned provides clarity and ensures that only necessary transaction properties are used during testing. Well done!
src/app/core/services/transaction.service.ts (2)
203-203:
Mind it! Replace hasOwnProperty for better safety.
Using Object.hasOwn(txn, 'source') is safer than txn.hasOwnProperty('source') as it avoids issues with objects that don't inherit from Object.prototype.
Apply this change:
-if (txn.hasOwnProperty('source') && Object.keys(txn).length === 1) {
+if (Object.hasOwn(txn, 'source') && Object.keys(txn).length === 1) {Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 203-203: 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)
198-212: 🛠️ Refactor suggestion
Hold on! Let's make this code more robust.
You're using hasOwnProperty directly on the object txn, which can lead to unexpected results if the object has been created without a proper prototype chain. It's safer to use Object.hasOwn() instead.
Additionally, consider adding type annotations to improve type safety and maintainability.
Apply this diff to make the changes:
const fileIds = fileObjs.map((fileObj) => fileObj.id);
if (fileIds.length > 0) {
return this.expensesService.createFromFile(fileIds[0], txn.source).pipe(
switchMap((result) => {
- if (txn.hasOwnProperty('source') && Object.keys(txn).length === 1) {
+ if (Object.hasOwn(txn, '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));
+ return this.upsert(this.cleanupExpensePayload(txn));
}
})
);Also, don't forget to import Object if needed.
Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 203-203: 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)
src/app/core/services/transaction.service.spec.ts (1)
1107-1112: 🧹 Nitpick (assertive)
Add an assertion to strengthen the test!
To ensure that expensesService.createFromFile is called with the correct parameters, consider adding an assertion in your test case.
Apply this addition to your test:
transactionService.createTxnWithFiles({ ...txnData }, of(mockFileObject)).subscribe((res) => {
expect(res).toEqual(txnData2);
+ expect(expensesService.createFromFile).toHaveBeenCalledOnceWith(mockFileObject[0].id, txnData.source);
expect(transactionService.upsert).toHaveBeenCalledOnceWith({
...txnDataCleaned,
file_ids: [fileObjectData1[0].id],
id: expenseData.id,
});This ensures that the createFromFile method is tested with the expected arguments.
Likely invalid or redundant comment.
src/app/core/mock-data/transaction.data.ts (1)
189-196: Well done! The addition brings clarity.
Introducing txnDataCleaned helps to ensure only necessary properties are included in transactions during tests, promoting better test practices.
|
|
||
| // 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; | ||
| } |
There was a problem hiding this comment.
🛠️ 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>;
}
arjunaj5
changed the title
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range comments (2)
src/app/fyle/add-edit-mileage/add-edit-mileage-3.spec.ts (2)
Line range hint
250-286: Mind it! The test case needs proper cloning of data.The test case now properly clones the unflattenedTxnData to prevent mutation, but there's a potential improvement.
Consider using a beforeEach block to set up the cloned data:
-spyOn(component, 'generateEtxnFromFg').and.returnValue(of(cloneDeep(unflattenedTxnData))); +let clonedTxnData; +beforeEach(() => { + clonedTxnData = cloneDeep(unflattenedTxnData); +}); +spyOn(component, 'generateEtxnFromFg').and.returnValue(of(clonedTxnData));
Line range hint
1-1000: Thalaiva says: Add test coverage for edge cases!The test suite could benefit from additional test cases covering edge scenarios.
Consider adding tests for:
- Network disconnection during expense submission
- Invalid report IDs
- Partial policy violation data
- Race conditions in async operations
Example test case structure:
it('should handle network disconnection during expense submission', (done) => { // Setup component.isConnected$ = new BehaviorSubject(true); // Simulate network disconnection setTimeout(() => { (component.isConnected$ as BehaviorSubject<boolean>).next(false); }, 100); // Test and assertions });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (5)
src/app/core/services/transaction.service.spec.ts(2 hunks)src/app/core/services/transaction.service.ts(2 hunks)src/app/fyle/add-edit-mileage/add-edit-mileage-3.spec.ts(8 hunks)src/app/fyle/add-edit-per-diem/add-edit-per-diem-3.page.spec.ts(11 hunks)src/app/fyle/add-edit-per-diem/add-edit-per-diem.page.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/app/core/services/transaction.service.ts
[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)
src/app/fyle/add-edit-per-diem/add-edit-per-diem-3.page.spec.ts
[error] 80-80: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.
(lint/suspicious/noFocusedTests)
🔇 Additional comments (4)
src/app/fyle/add-edit-per-diem/add-edit-per-diem.page.ts (2)
1983-1985: Kabali style! The transaction sync looks good.
The parameters for addEntryAndSync are properly structured with transaction, dataUrls, and comments. This is how you write clean code!
1974-1979:
Mind it! Potential null reference issue in report ID assignment.
The condition formValue.report && (etxn.tx.policy_amount === null || (etxn.tx.policy_amount && !(etxn.tx.policy_amount < 0.0001))) is checking policy amount, but not verifying if formValue.report.id exists. When I say something exists, it better exist!
Here's how you fix it, like a boss:
-if (formValue.report && (etxn.tx.policy_amount === null || (etxn.tx.policy_amount && !(etxn.tx.policy_amount < 0.0001)))) {
- transaction.report_id = formValue.report.id;
+if (
+ formValue.report?.id &&
+ (etxn.tx.policy_amount === null || (etxn.tx.policy_amount && !(etxn.tx.policy_amount < 0.0001)))
+) {
+ transaction.report_id = formValue.report.id;Likely invalid or redundant comment.
src/app/core/services/transaction.service.ts (1)
198-212: 🛠️ Refactor suggestion
Listen up, partner! Let's make this code more stylish than my signature walk!
The code needs some style upgrades in the following areas:
- Replace
hasOwnPropertywithObject.hasOwn()for better safety - Add proper type annotations for better type safety
Here's how to make it super:
return fileUploads$.pipe(
switchMap((fileObjs) => {
const fileIds = fileObjs.map((fileObj) => fileObj.id);
const isReceiptUpload = txn.hasOwnProperty('source') && Object.keys(txn).length === 1;
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 {
return this.upsert(txn);
}
})
);Likely invalid or redundant comment.
🧰 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)
src/app/core/services/transaction.service.spec.ts (1)
44-44: LGTM! Import statement added with style!
The import of txnDataCleaned is perfectly placed with other transaction data imports.
| [] | ||
| ); | ||
| done(); |
There was a problem hiding this comment.
🧹 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
| 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, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
🛠️ 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
| // 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; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mind it! Let's add some punch to this code with proper types!
The method needs some power-packed improvements:
- Add proper return type annotation
- Use type-safe implementation
- 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>;
}| @@ -77,7 +77,7 @@ import { expectedReportsPaginated } from 'src/app/core/mock-data/platform-report | |||
| import { PerDiemRedirectedFrom } from 'src/app/core/models/per-diem-redirected-from.enum'; | |||
|
|
|||
| export function TestCases3(getTestBed) { | |||
| return describe('add-edit-per-diem test cases set 3', () => { | |||
| return fdescribe('add-edit-per-diem test cases set 3', () => { | |||
There was a problem hiding this comment.
Hey! Don't focus on just one test, let all tests shine like stars!
Using fdescribe prevents other tests from running. This is typically used during development and should be removed before committing.
Replace fdescribe with describe:
- return fdescribe('add-edit-per-diem test cases set 3', () => {
+ return describe('add-edit-per-diem test cases set 3', () => {📝 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.
| return fdescribe('add-edit-per-diem test cases set 3', () => { | |
| return describe('add-edit-per-diem test cases set 3', () => { |
🧰 Tools
🪛 Biome (1.9.4)
[error] 80-80: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.
(lint/suspicious/noFocusedTests)
| expensesService.createFromFile.and.returnValue(of({ data: [expenseData] })); | ||
| 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, | ||
| }); |
There was a problem hiding this comment.
🧹 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();
}
});
});…ile-app into Fyle-expenses-post-fix_2
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
src/app/fyle/add-edit-per-diem/add-edit-per-diem-3.page.spec.ts(10 hunks)
🔇 Additional comments (2)
src/app/fyle/add-edit-per-diem/add-edit-per-diem-3.page.spec.ts (2)
617-623: Super! Defining 'expectedEtxnDataWithReportId' brings clarity to your tests. Keep rocking!
625-625: Well done! Mocking 'etxn$' with 'cloneDeep' makes your tests strong as steel. Fantastic!
| spyOn(component, 'criticalPolicyViolationErrorHandler').and.returnValue( | ||
| of({ etxn: cloneDeep(unflattenedTxnData) }) | ||
| ); | ||
| spyOn(component, 'policyViolationErrorHandler').and.returnValue( | ||
| of({ etxn: unflattenedTxnData, comment: 'comment' }) | ||
| of({ etxn: cloneDeep(unflattenedTxnData), comment: 'comment' }) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Why duplicate when you can simplify? Refactor those 'spyOn' calls and make your code unstoppable!
You're repeating spyOn for criticalPolicyViolationErrorHandler and policyViolationErrorHandler with similar return values. Extract this repetitive code into a shared setup to streamline your tests.
| expectedEtxnDataWithReportId.tx, | ||
| undefined, | ||
| [], | ||
| 'rprAfNrce73O' | ||
| [] |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Too much repetition can slow you down! Consolidate those test assertions and keep your code swift and powerful!
You're calling transactionOutboxService.addEntryAndSync with similar parameters in multiple tests. Consider abstracting these calls or using parameterized tests to enhance maintainability and reduce code duplication.
Also applies to: 724-726, 769-771, 812-814, 851-853, 895-897, 929-931
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
src/app/core/services/transaction.service.ts(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/app/core/services/transaction.service.ts
[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)
🔇 Additional comments (2)
src/app/core/services/transaction.service.ts (2)
198-212: Superstar code right here! The logic is solid as a rock!
The enhanced flow for handling receipt uploads and expense creation is well-structured:
- Extracts file IDs efficiently
- Handles receipt uploads intelligently
- Updates expense with cleaned data when needed
🧰 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)
929-938: 🛠️ Refactor suggestion
Style ah? Let's add some punch to this code with proper types!
The method needs some power-packed improvements:
- Add proper return type annotation
- Use type-safe implementation
- Consider using Object.entries for better readability
Here's the mass-u implementation:
- 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>;
}| .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; |
There was a problem hiding this comment.
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.
| 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)
|
Clickup
https://app.clickup.com/t/86cx8j66c
Code Coverage
Please add code coverage here
UI Preview
Please add screenshots for UI changes
Summary by CodeRabbit
New Features
Improvements