-
Notifications
You must be signed in to change notification settings - Fork 1
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
MPDX-8318 Financial Accounts Summary and Transactions pages #1127
Conversation
Bundle sizes [mpdx-react]Compared against 14063e9
|
1b95620
to
9299d6b
Compare
Preview branch generated at https://MPDX-8318.d3dytjb8adxkk5.amplifyapp.com |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really great work so far in such a quick time! You might be aware of some of these already, but here are some bugs/discrepancies I noticed. Let me know if I can clarify any of them:
I used the "RUSSIA ALL Ministries RC (local + Europe)" account list in my testing.
- The accounts should be sorted alphabetically on the main financial accounts list
- Crossroads Samara is unchecked in Angular and checked in React
- Last synced dates are in the past in Angular but are all today in React
- Graph and average is different for Crescendo in Angular vs React
- In Summary, date links in header and category line items aren't clickable links
- In Summary for Crescendo, "Nonlocal contr assessments-RUS" numbers are all negative in Angular but are positive in React
- In Transactions, oldest transactions are at the bottom in Angular but are at the top in React
- In Transactions, search placeholder doesn't match the tooltip
- In Transactions CSV export, the transactions are also reversed
- In Transactions CSV export, the currency amounts include
\xA0
characters instead of regular spaces, which causes those cells to render strangely in Excel - After clicking on an account there's no way to go back to the financial accounts list
src/components/Reports/FinancialAccountsReport/Header/Header.tsx
Outdated
Show resolved
Hide resolved
src/components/Reports/FinancialAccountsReport/Header/Header.tsx
Outdated
Show resolved
Hide resolved
pages/accountLists/[accountListId]/reports/financialAccounts/[[...financialAccount]].page.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, really great work with this! Feel free to push back on any of my comments or wait until later to do them since I know we're trying to get this out this week.
pages/api/Schema/reports/financialAccounts/financialAccounts/financialAccounts.graphql
Show resolved
Hide resolved
src/components/Reports/FinancialAccountsReport/Context/FinancialAccount.graphql
Show resolved
Hide resolved
return ( | ||
<StyledTableCell | ||
key={`income-${idx}`} | ||
align="right" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't need styles since it's empty, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thi table row (Opening Balance) isn't empty, I assume you mean the Income table row. This table row needs styles and empty rows to go all away across the table
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry if my comment showed up in a weird place and wasn't clear. Yes, I meant the income header. I'm asking why we have align="right"
on this cell and the expenses cell since the cell doesn't have any content?
return ( | ||
<StyledTableCell | ||
key={`expenses-${idx}`} | ||
align="right" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also empty
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This table row needs styles and empty rows to go all away across the table
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This table row needs styles and empty rows to go all away across the table
Definitely! I'm just wondering why we have align="right"
on an empty cell.
src/components/Reports/FinancialAccountsReport/AccountTransactions/AccountTransactions.tsx
Outdated
Show resolved
Hide resolved
pages/accountLists/[accountListId]/reports/financialAccounts/[[...financialAccount]].page.tsx
Show resolved
Hide resolved
src/components/Reports/FinancialAccountsReport/FinancialAccounts/FinancialAccounts.tsx
Outdated
Show resolved
Hide resolved
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incredible work with all of this!
The only other thing I noticed is that the Export CSV button doesn't do anything while the transactions are loading. Can we disable it?
src/components/Reports/FinancialAccountsReport/AccountSummary/AccountSummaryHelper.ts
Outdated
Show resolved
Hide resolved
src/components/Reports/FinancialAccountsReport/AccountSummary/AccountSummaryHelper.ts
Outdated
Show resolved
Hide resolved
src/components/Reports/FinancialAccountsReport/AccountTransactions/AccountTransactions.tsx
Outdated
Show resolved
Hide resolved
src/components/Reports/FinancialAccountsReport/AccountTransactions/AccountTransactions.tsx
Show resolved
Hide resolved
src/components/Reports/FinancialAccountsReport/AccountTransactions/AccountTransactions.tsx
Outdated
Show resolved
Hide resolved
src/components/Reports/FinancialAccountsReport/AccountTransactions/AccountTransactions.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some failing tests, but this looks really good! Amazing work! 🚀
5. Adding links from summary to transactions this will break some tests, but I will fix them tomorrow.
I will create a new PR which adds all the tests for the Transactions page and components I have missed the tests due to the timeline of getting this out on production. |
Description
In this PR I add the Financial Accounts Summary and Transactions pages. There is a lot I do in this PR, but there are a lot of files which are just renamed.
Renamed "Responsibility Center" to "Financial Accounts" - The reason is that these pages allow you to view details on a certain Financial Account. Which is what the old Angular version had in the code and URL. "Responsibility Center" is the name by which users know it, but for us devs, it's known as "Financial Accounts", so to keep it consistent for us, I've renamed it.
I added a new context around the Financial Accounts pages since there's a lot of functionality that needs to be passed from parent to component.
Caleb and I created the framework that the two pages would sit on, this includes the Header and reports navigation.
I added the Summary page - This is the simplest of the 2, and shows a table without any filtering. This table should have links to the transaction table, which will be coming today. I haven't had time to build the links, but it should be fairly easy.
Things I still need to do:
As time is running out, since we are putting this live on Friday, I wanted to get this PR up, so people can review it, while I still work on the following items.
Checklist: