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

feat: Move My Expenses to Platform: Moved API Call Of Expenses in My Expenses And Updated Mapping In Expenses Card #2542

Merged
merged 39 commits into from
Dec 5, 2023

Conversation

jayfyle
Copy link
Contributor

@jayfyle jayfyle commented Nov 7, 2023

Description

🤖 Generated by Copilot at be953d5

This pull request migrates the app from using the old expense model to the new platform expense model. It updates the transaction service, the my expenses page, and the expenses card component to use the new model and api service. It also adds new models and interfaces for platform expenses, categories and cost centers.

🤖 Generated by Copilot at be953d5

To use the new platform expense model
We had to make changes quite colossal
We updated the service
And the component with purpose
And imported the models and protocol

Walkthrough

🤖 Generated by Copilot at be953d5

  • Add new models and interfaces for platform expenses, categories and cost centers (link, link, link, link, link)
  • Inject the spender platform api service in the transaction service and define new methods for getting platform expenses and their count (link, link)
  • Replace the old expense model with the new platform expense model in the my expenses page and the expenses card component (link, link, link, link)
  • Remove unused imports and code from the my expenses page and the expenses card component (link, link, link)
  • Add new behavior subjects for loading platform expenses and their parameters in the my expenses page (link)
  • Initialize the load expenses behavior subject with default values in the my expenses page (link)
  • Use the new methods and models for getting platform expenses and their count, and the new query parameters in the my expenses page (link)
  • Use the new behavior subject for loading platform expenses instead of the old one in the my expenses page (link, link, link)
  • Use the new model properties for platform expenses instead of the old expense model properties in the my expenses page and the expenses card component (link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link)
  • Add new properties and methods for checking the draft and policy violation status, and getting the vendor details of the platform expense in the expenses card component (link)

Clickup

https://app.clickup.com/t/86ctv3kk4

Code Coverage

Please add code coverage here

UI Preview

Please add screenshots for UI changes

suyashpatil78 and others added 2 commits November 7, 2023 10:45
* fix: Per Diem scan fix and redirection fix for view report

* small fix

* pr comments
@github-actions github-actions bot added the size/L Large PR label Nov 7, 2023
Copy link

github-actions bot commented Nov 7, 2023

PR title must start with "fix:" or "feat:" or "test:"

1 similar comment
Copy link

github-actions bot commented Nov 7, 2023

PR title must start with "fix:" or "feat:" or "test:"

@jayfyle
Copy link
Contributor Author

jayfyle commented Nov 7, 2023

Please ignore the unit tests, for now, I will fix them in the next PR. As of now only the logic for fetching the expenses and displaying them using the expense cards is updated. The logic for filtering and searching will be moved in the upcoming PRs, and subsequently, the PRs will be shorter since the major portion of the API call is done in this one.

@jayfyle jayfyle changed the title Moved API Call Of Expenses in My Expenses And Updated Mapping In Expenses Card feat: Moved API Call Of Expenses in My Expenses And Updated Mapping In Expenses Card Nov 7, 2023
@@ -90,18 +91,22 @@ export class MyExpensesPage implements OnInit {

isConnected$: Observable<boolean>;

myExpenses$: Observable<Expense[]>;
myExpenses$: Observable<PlatformExpense[]>;

count$: Observable<number>;

isInfiniteScrollRequired$: Observable<boolean>;

loadData$: BehaviorSubject<Partial<GetExpensesQueryParamsWithFilters>>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will remove this property when working on filtering

@jayfyle jayfyle marked this pull request as ready for review November 7, 2023 06:24
Copy link
Contributor

@OmkarJ13 OmkarJ13 left a comment

Choose a reason for hiding this comment

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

Can you check latest master, I have merged a few changes which add the expense models and services, you can use those directly

@jayfyle jayfyle force-pushed the move-platform-my-exp branch from f593a16 to 979143a Compare November 7, 2023 12:08
@@ -79,6 +78,9 @@ import { UniqueCards } from 'src/app/core/models/unique-cards.model';
import { CategoriesService } from 'src/app/core/services/categories.service';
import { PlatformCategory } from 'src/app/core/models/platform/platform-category.model';
import { ReportV1 } from 'src/app/core/models/report-v1.model';
import { PlatformGetExpenseQueryParam } from 'src/app/core/models/platform/platform-get-expenses-query.model';
import { ExpensesService } from 'src/app/core/services/platform/v1/spender/expenses.service';
import { Expense as PlatformExpense } from 'src/app/core/models/platform/v1/expense.model';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

using an alias right now to avoid errors, will remove it once the entire page is migrated

@jayfyle jayfyle requested a review from OmkarJ13 November 7, 2023 12:56
Copy link
Contributor

@OmkarJ13 OmkarJ13 left a comment

Choose a reason for hiding this comment

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

added comments

Comment on lines 1 to 10
export interface ExpenseParams {
split_group_id?: string;
employee_id?: string;
state?: string;
or?: string | string[];
report_id?: string;
order?: string;
scalar?: boolean;
id?: string;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@arjunaj5 Is also adding a similar model, can you check with him to avoid conflicts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Merged latest master and using the pre-existing models and services

Copy link
Contributor

Choose a reason for hiding this comment

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

Added a comment about this model here.

Comment on lines 92 to 95
source_account: {
id: string;
type: AccountType;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Didn't follow this change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was trying to fix some type error, no longer required, reverting back.

src/app/core/services/transaction.service.ts Outdated Show resolved Hide resolved
src/app/fyle/my-expenses/my-expenses.page.ts Outdated Show resolved Hide resolved
src/app/fyle/my-expenses/my-expenses.page.ts Outdated Show resolved Hide resolved
@@ -79,7 +79,7 @@
class="expenses-card--receipt-container expenses-card--with-image"
[ngStyle]="{
'background-image':
expense.tx_dataUrls?.length > 0 &&
Copy link
Contributor

@OmkarJ13 OmkarJ13 Nov 7, 2023

Choose a reason for hiding this comment

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

tx_dataUrls was manually added by us I think in the frontend, can we safely replace this with file_ids?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, if I can recall we generated this on our end to get all file URLs for receipts, although this key looks similar, we have to use it for now, until the proper key can be determined

Copy link
Contributor

@OmkarJ13 OmkarJ13 Nov 8, 2023

Choose a reason for hiding this comment

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

Sure, pls add a TODO to make sure we don't miss this if needed

Suraj Kumar and others added 2 commits November 7, 2023 19:18
…s://app.clickup.com/t/85ztztwg6 (#2539)

* fix: Added fix for Item not in list: https://app.clickup.com/t/85ztztwg6

* fix: console.log removed

* fix: Hack for diaplay_name field

* fix: moved the exception to a seprate function

* feat: Added Display Name Additional condition

* fix; comment added

* feat: added tests

* test: UT fix

* test: test file formatting

* test: text change

* feat: added any type in template ref

* fix: review based typing

* fix: typing added to noop methods

* feat: typing fix

* feat: comment updated
@jayfyle jayfyle requested a review from OmkarJ13 November 7, 2023 14:34
Copy link
Contributor

@OmkarJ13 OmkarJ13 left a comment

Choose a reason for hiding this comment

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

Some more comments

Comment on lines 27 to 30
offset: number;
limit: number;
order: string;
queryParams: ExpenseParams;
Copy link
Contributor

Choose a reason for hiding this comment

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

This whole thing can be a model

queryParams: ExpenseParams;
}>
): Observable<PlatformApiResponse<Expense>> {
return from(this.authServie.getEou()).pipe(
Copy link
Contributor

Choose a reason for hiding this comment

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

authService

src/app/core/services/transaction.service.ts Outdated Show resolved Hide resolved
@@ -19,4 +21,35 @@ export class ExpensesService {

return this.spenderService.get<PlatformApiResponse<Expense>>('/expenses', data).pipe(map((res) => res.data[0]));
}

getExpenses(
Copy link
Contributor

Choose a reason for hiding this comment

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

Will these methods be not used by the approver persona? (team pages)
You can add them under approver/expenses.service.ts too if needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will add them later

params: {
offset: config.offset,
limit: config.limit,
employee_id: `eq.${eou.ou.id}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this needed? In platform the spender API will by default return the user's expenses only

Copy link
Contributor Author

@jayfyle jayfyle left a comment

Choose a reason for hiding this comment

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

Merged master and a lot of changes I added were already done by others, so I removed my methods and used the services and models already on master.

@jayfyle jayfyle changed the title feat: Moved API Call Of Expenses in My Expenses And Updated Mapping In Expenses Card feat: Move My Expenses to Platform: Moved API Call Of Expenses in My Expenses And Updated Mapping In Expenses Card Nov 14, 2023
Comment on lines 1 to 10
export interface ExpenseParams {
split_group_id?: string;
employee_id?: string;
state?: string;
or?: string | string[];
report_id?: string;
order?: string;
scalar?: boolean;
id?: string;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Added a comment about this model here.

@jayfyle jayfyle requested a review from Dimple16 November 17, 2023 08:18
Copy link
Contributor

@Dimple16 Dimple16 left a comment

Choose a reason for hiding this comment

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

2 reviewed have checked, will skip

jayfyle and others added 2 commits November 30, 2023 20:54
#2546)

* fixed expense navigation and search

* reverted dates on pending transaction list

* feat: Move My Expenses to Platform: Fixing Select Functionality and Create Report From List (#2557)

* fixed select expense and add to report

* fixed select all

* changed create-report card

* removed commented code

* resolved comments

* feat: Move My Expenses to Platform: Fixing Filters and Sorting  (#2562)

* fixed filters and sorting

* resolved comments

* feat: Move My Expenses to Platform: Fixed Delete and Card Filters (#2569)

* fixed delete and card sorting

* resolved comments

* feat: Move My Expenses to Platform: Fixing Filters on Refresh and Broken Filters (#2570)

* fixing filter refresh and broken filters

* resolved comments

* fixed typo

* feat: Move My Expenses to Platform: Fixed Stats (#2571)

* fixed stats

* added cloneDeep

* resolved comments

* feat: Fixing Merge and Search Refresh on Platform My Expenses (#2581)

* fixing merge -1

* fixed search params

* fixed extra card on empty search and cancel search

* fixed merge expense bug

* fixed create report from my expenses

* resolved comments

* test: Move My Expenses To My Platform: Fixing Types and Errors (#2586)

* fixed all types issues and errors

* resovled comments

* test: Move My Expenses To Platform: Fixing tests on Merge Expenses and Create New Report Component (#2587)

* fixed test for merge expense and create report modal

* fixed ionviewwillenter test in merge

* resolved comments

* test: Move My Expenses to Platform: Fixing tests on My Expenses Service (#2588)

* fixed my expenses service

* resolved comments

* test: Move My Expenses To Platform: Fixing Test on My Expenses Part 1 (#2590)

* fixed my expenses

* removed fdescribe

* test: Move My Expenses to Platform:  Fixing Test on My Expenses Part 2 (#2596)

* fixed tests for other methods

* resolved comments

* test: Move My Expenses to Platform: Fixed IonViewWillEnter tests (#2597)

* fixed all tests

* feat: Move My Expenses to Platform: Fixing Review Expenses (#2598)

* fixed review expenses

* test: Move My Expenses to Platform: Fixing OpenReviewExpenses Tests (#2599)

* fixing openreviewexpenses tests

* resovled comments

* test: Move My Expenses to Platform: Fixed All Tests (#2601)

* fixed all tests

* removed optional chaining

* resolved comments

* fixed broken test

* fixing failing test -1

* fixing test -3

* added data

* feat: Move My Expenses to Platform: Added Public My Expenses And Renamed Platform to V2 (#2602)

* added public my-expenses page

* removed unrequired files

* updated imports

* feat: Move My Expenses to Platform: Fixed Card Filter Param (#2603)

* fixed card params

* feat: Move My Expenses To Platform: Added Route Guard for redirection And Updated Model (#2607)

* added guard and updated model

* added test and resolved comments

---------

Co-authored-by: Jay Budhadev <[email protected]>

---------

Co-authored-by: Jay Budhadev <[email protected]>

---------

Co-authored-by: Jay Budhadev <[email protected]>

---------

Co-authored-by: Jay Budhadev <[email protected]>

---------

Co-authored-by: Jay Budhadev <[email protected]>

---------

Co-authored-by: Jay Budhadev <[email protected]>

---------

Co-authored-by: Jay Budhadev <[email protected]>

---------

Co-authored-by: Jay Budhadev <[email protected]>

---------

Co-authored-by: Jay Budhadev <[email protected]>

---------

Co-authored-by: Jay Budhadev <[email protected]>

---------

Co-authored-by: Jay Budhadev <[email protected]>

---------

Co-authored-by: Jay Budhadev <[email protected]>

---------

Co-authored-by: Jay Budhadev <[email protected]>

---------

Co-authored-by: Jay Budhadev <[email protected]>

---------

Co-authored-by: Jay Budhadev <[email protected]>

---------

Co-authored-by: Jay Budhadev <[email protected]>

---------

Co-authored-by: Jay Budhadev <[email protected]>

---------

Co-authored-by: Jay Budhadev <[email protected]>

---------

Co-authored-by: Jay Budhadev <[email protected]>
@github-actions github-actions bot added size/XL Extra Large PR and removed size/M Medium PR labels Nov 30, 2023
Copy link

github-actions bot commented Dec 5, 2023

Unit Test Coverage % values
Statements 97.07% ( 19316 / 19899 )
Branches 95.03% ( 9854 / 10369 )
Functions 96.07% ( 5926 / 6168 )
Lines 97.11% ( 18385 / 18932 )

@jayfyle jayfyle merged commit 05df977 into master Dec 5, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/XL Extra Large PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants