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: Allow save expense with invalid fields after capture receipt #3308

Merged
merged 12 commits into from
Dec 9, 2024
Merged

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

Changes from 2 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
253d674
make create_from_file call if capture receipt
arjunaj5 Dec 6, 2024
ca1b84b
minor
arjunaj5 Dec 6, 2024
467025e
minor
arjunaj5 Dec 6, 2024
4809082
minor
arjunaj5 Dec 6, 2024
436e0ef
minor refactor
arjunaj5 Dec 6, 2024
469dea5
minor refactor and fix tests
arjunaj5 Dec 6, 2024
02fb34e
modify createTxnFromFile to make 2 API calls only if amount is not pr…
arjunaj5 Dec 8, 2024
ce96a7f
fix failing tests
arjunaj5 Dec 8, 2024
4cf8b8f
Merge branch 'master' into Fyle-expenses-post-fix_2
arjunaj5 Dec 9, 2024
8ee656f
minor
arjunaj5 Dec 9, 2024
32abf93
Merge branch 'Fyle-expenses-post-fix_2' of github.com:fylein/fyle-mob…
arjunaj5 Dec 9, 2024
71c5167
minor bug fix
arjunaj5 Dec 9, 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
35 changes: 25 additions & 10 deletions src/app/core/services/transaction.service.ts
View file
Open in desktop
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
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));
}
})
);
Copy link

Choose a reason for hiding this comment

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

🛠️ 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:

  1. Replace hasOwnProperty with Object.hasOwn() for better safety
  2. 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.

Suggested change
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)

} else {
const fileIds = fileObjs.map((fileObj) => fileObj.id);
txn.file_ids = fileIds;
return this.upsert(txn);
}
})
Expand Down Expand Up @@ -919,4 +924,14 @@

return typeOrFilter;
}

private cleanupExpenseCreatePayload(txn: Partial<Transaction>) {

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

View workflow job for this annotation

GitHub Actions / Run linters 

Missing return type on function

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

View workflow job for this annotation

GitHub Actions / Run linters 

Missing return type on function
const newTxn = {};
for (const key in txn) {
if (txn[key] !== null && txn[key] !== undefined) {
newTxn[key] = txn[key];

Check failure on line 932 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 932 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;
}
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 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

}
Loading