-
Notifications
You must be signed in to change notification settings - Fork 3.5k
feat: [CFI] Add group-by report layout feature #76169
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: [CFI] Add group-by report layout feature #76169
Conversation
|
Hey, I noticed you changed If you want to automatically generate translations for other locales, an Expensify employee will have to:
Alternatively, if you are an external contributor, you can run the translation script locally with your own OpenAI API key. To learn more, try running: npx ts-node ./scripts/generateTranslations.ts --helpTypically, you'd want to translate only what you changed by running |
|
@shubham1206agra Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
|
🚧 @mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
|
🚧 @mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
JmillsExpensify
left a comment
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.
Looks good from a product perspective.
src/components/MoneyRequestReportView/MoneyRequestReportTransactionList.tsx
Outdated
Show resolved
Hide resolved
src/components/MoneyRequestReportView/MoneyRequestReportTransactionList.tsx
Show resolved
Hide resolved
src/components/MoneyRequestReportView/MoneyRequestReportGroupHeader.tsx
Outdated
Show resolved
Hide resolved
src/components/MoneyRequestReportView/MoneyRequestReportGroupHeader.tsx
Outdated
Show resolved
Hide resolved
mountiny
left a comment
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.
@TaduJR Changes look good, but why are there no unit tests? All the helper method and actions for the api should be covered with extensive unit testing. @shubham1206agra you should not be approving prs that do not have the tests ready for future.
@TaduJR please add the tests for all the methods and actions in a follow up
| // Exclude IOU reports - only show for workspace expense reports | ||
| if (isIOUReportUtils(report)) { | ||
| return false; | ||
| } |
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.
why do you need this if you have that
if (!isExpenseReportUtils(report)) {
return false;
}
Check in place already
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.
isExpenseReport is true for all money request reports (including IOU/DM), and we explicitly need to exclude IOU. So we need both.
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.
Unit tests
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.
Unit tests
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.
unit tests
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.
All of these methods should be unit tested
|
JUST TO NOTE: We are missing the “group by” label, and I feel the offline scenarios are not aligned in one edge case. For example, we have three transactions but two of them have a pending delete status — in this case, the grouping is still shown and we also show the “Report layout” option. I’m not sure what the expected behavior should be, but I think it’s an edge case and not a blocker for now. |
Hey @mountiny. I already asked to continue working on unit tests but I thought they were not needed since the feature was needed immediately. Even asked here before #72943 (comment) |
@Krishna2323 I fixed that incase you missed it |
|
@TaduJR thanks for the context, lets follow up with the tests soon 🙌 thank you |
Thanks will do that. As I told you before I thought they were not needed after my comment here #72943 (comment) got jumped accidentally. |
Ok, but I am not sure why would you not include them in the PR if you already had some written? Better to have some than none |
Thats because I was not sure about all the unit tests are doing whats expected, since I choose to make the feature perfect then write the test then after some rush it got merged But will choose TDD after now. |
|
Sounds great, next, lets focus on any blockers that might arise to fix them quickly or demote tomorrow. Thanks for the hard work! |
|
Awesome |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.2.65-0 🚀
|
|
That is intentional for this v1 to get it out asap |
|
Oh hm, is there a PR for the v2? 😕 |
|
Yea Unfortunately the convertedAmount field was a bit problematic, then we decided to got without subtotal and hold it in v2. Ref: https://expensify.slack.com/archives/C01GTK53T8Q/p1764158298659759 |
|
🚀 Deployed to production by https://github.com/marcaaron in version: 9.2.65-6 🚀
|
Explanation of Change
This PR implements a group-by report layout feature that allows users to organize expense report transactions by Category or Tag, matching the familiar OldDot functionality that NewDot customers have been requesting.
Fixed Issues
$ #72100
$ #76079
$ #76082
$ #76084
$ #76085
$ #76087
$ #76091
PROPOSAL: #72100 (comment)
Tests
Desktop Testing
Category Grouping:
Tag Grouping:
Preference Persistence:
Offline Mode:
Edge Cases:
Mobile Testing (iOS/Android)
Category Grouping:
Group Selection:
Both Platforms Desktop and Mobile:
Precondition:
Have a very long category - Home Office Home Office Home Office Home Office Home Office Home Office Home Office Home Office Home Office Home Office Home Office Home Office Home Office Home Office Home Office Home Office Home Office Home Office Home Office Home Office Home Office H
Go to staging.new.expensify.com
Go to workspace chat.
Create an expense with car category.
Create an expense with a very long category.
Open the expense report.
Verify Long category should be truncated.
Go to staging.new.expensify.com
Go to workspace chat.
Create an expense without category.
Open the expense report.
Go offline.
Create another expense without category.
Go online.
Verify Expense without category should be grouped as "Uncategorized" when created offline.
Go to staging.new.expensify.com
Go to workspace chat.
Create three expenses with different amount and category (category should not in alphabetical order).
Open the expense report.
Open the first expense.
Navigate through the expenses with arrow on the top right.
Verify Navigation via arrows should sync with the order of expenses in the expense report.
Precondition:
Precondition:
Workspace has multi-level independent tags.
Go to staging.new.expensify.com
Go to workspace chat.
Create an expense with all tags selected.
Create another expense with only the second tag selected.
Open the expense report.
Click More > Report layout > Tag.
Verify Each tag will be followed by a comma.
Verify There will be no colon in front of the tag if the previous tag is not selected.
Go to staging.new.expensify.com
Go to workspace chat.
Create an expense without category.
Open the expense report.
Go offline.
Create another expense without category.
Go online.
Verify Expense without category should be grouped as "Uncategorized" when created offline.
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari