-
Notifications
You must be signed in to change notification settings - Fork 3k
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: Expense Tracking in the Workspace Chat #39239
feat: Expense Tracking in the Workspace Chat #39239
Conversation
I've created this draft PR to start tracking the existing issues that are blocking the PR, but are not directly related to it. |
First of all, the App is crashing because of the Shortcut feature – it doesn't support the
@thienlnam could you please help with adding the corresponding label or tagging relevant people? |
Another issue – the removal doesn't work as expected. It removes the action optimistically, but after a refresh, the action reappears as a skeleton, and the transaction thread exists, but with an empty transaction. @shubham1206agra is it known from the times you implemented the self-dm tracking? Screen.Recording.2024-03-29.at.14.20.2114.22.mp4 |
Yes it's a known issue |
No, as we don't have any such thing in docs. But let me summon @Expensify/design so that they can chime in. |
This is a non-reimbursable expense. That's why it will behave like this. |
Not sure. @thienlnam can confirm this. |
Again @Expensify/design can help here. |
Hmm, I'm really curious for the other designer's thoughts on these.
For this one, I could maybe see editing the
This one has me kinda stumped. I don't know that we want to change the preview behavior or create a custom preview for tracked expenses, so I'm not sure if there's a good solution for this. @Expensify/design any ideas? |
Correct - whether you track a personal expense or send someone a money request, they are both just expenses under the hood and they would look the same at the expense level.
I don't understand why we are handling it this way. If you add an expense to a report, it just increases the total. The "Company spend" vs "Out of pocket spend" is just a nicer way of saying "Is this expense reimburseable or not?" Unless I am missing something. Did the GH issue or design doc specify otherwise?
Yes. We already have existing patterns for what expenses look like when they are added to a workspace, so we just follow what we currently have. @thienlnam can you also chime in here? Seems like maybe there is some misunderstanding going on. |
Yes, from what I understood. |
One more BE issue related to money request removal:
The Screen.Recording.2024-03-30.at.16.46.2716.47.mp4 |
@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] |
@shubham1206agra you can start the preliminary testing (just please be aware that the current |
@paultsimura @Expensify/design Should we show paid sign to non-reimbursable request? |
Hmm did the user take any action to trigger that state? Like did the user mark the report as Paid? If so, then yes. |
Yes, I used |
@thienlnam this must be a single-transaction Expense Report. |
Ah gotcha, this seems to happen for regular requests as well - I will create an issue to track it |
Thanks, I will double check tomorrow if I can fix the skeleton bug without this BE fix |
# Conflicts: # src/pages/home/report/ReportActionsView.tsx
I couldn't reproduce for the regular ones 🤔 |
I managed to localize the issue more:
An observation: the TrackExpense API response turns the empty IOU preview into a "deleted" empty action: However, it doesn't re-link the new report preview action with the action that came before the deleted empty preview. As a result, this function, which builds the report actions list by linking them via App/src/libs/ReportActionsUtils.ts Lines 281 to 340 in 20be55b
My suggestion is to fix the Expense Report removal on the BE side rather than find workarounds. |
Yeah I agree we shouldn't find a workaround from the FE side. However, it doesn't seem like a blocker so we can proceed here and then once the BE is fixed it should just work |
@shubham1206agra the longer it takes to review & merge this PR – the more conflicts arise (new ones each day). As per the last comment, this PR should be ready for the final testing |
# Conflicts: # tests/unit/ReportUtilsTest.ts
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeiOS: NativeiOS: mWeb SafariScreen.Recording.2024-04-08.at.6.38.56.PM.mp4MacOS: Chrome / SafariScreen.Recording.2024-04-08.at.6.12.58.PM.mp4MacOS: Desktop |
@thienlnam The track is taking longer to fetch from BE. (Little confused here tbh) |
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.
LGTM!
# Conflicts: # src/libs/ReportUtils.ts
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.
Great - thanks!
Perf tests known to be failing |
✋ 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/thienlnam in version: 1.4.62-0 🚀
|
1 similar comment
🚀 Deployed to staging by https://github.com/thienlnam in version: 1.4.62-0 🚀
|
🚀 Deployed to staging by https://github.com/thienlnam in version: 1.4.62-0 🚀
|
We seem to have a regression here #40067. If anyone can please take a look cc @paultsimura @shubham1206agra @thienlnam |
I'm on it |
🚀 Deployed to production by https://github.com/thienlnam in version: 1.4.62-17 🚀
|
Details
This PR enables tracking expenses within the Workspace money reports.
Fixed Issues
$ #38971
PROPOSAL: N/A
Tests
Same as QA
Offline tests
Same as QA
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
android20.52.mp4
Android: mWeb Chrome
chrome20.45.mp4
iOS: Native
Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-04-01.at.20.26.5220.28.mp4
iOS: mWeb Safari
Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-04-01.at.20.37.5120.38.mp4
MacOS: Chrome / Safari
Screen.Recording.2024-04-01.at.19.46.3119.47.mp4
MacOS: Desktop
Screen.Recording.2024-04-01.at.19.57.2519.58.mp4