-
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
Fix - Submit expense RHP doesn't include workspaces from Track flow #52061
Fix - Submit expense RHP doesn't include workspaces from Track flow #52061
Conversation
@hoangzinh 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] |
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2025-01-31.at.14.16.36.android.mp4Android: mWeb ChromeScreen.Recording.2025-01-31.at.14.21.17.moviOS: NativeScreen.Recording.2025-01-23.at.21.33.41.ios.moviOS: mWeb SafariScreen.Recording.2025-01-23.at.21.35.40.ios.safari.movMacOS: Chrome / SafariScreen.Recording.2025-01-23.at.17.36.44.web.movMacOS: DesktopScreen.Recording.2025-01-23.at.17.42.56.desktop.mov |
Bug 1: Unable to submit Expense to workspace from Track flow
Screen.Recording.2024-11-06.at.16.20.03.mov@FitseTLT are you facing same experience as me? If yes can you help to check whether it's a FE or BE issue? I suspect it might be related to those request params |
Yeah @hoangzinh you are right but I think it is a BE bug
@luacmartins We need your help on this ^ one. Thx. |
@thienlnam can you confirm if |
No, there probably need to be some additional changes made to support that use case |
@FitseTLT thank you so much for your thoughtful investigation. |
@thienlnam do you think you could work on the needed Auth changes since you're familiar with the track flow? |
I really don't have the capacity to take on that kind of change right now unfortunately |
@luacmartins should we find another internal engineer to help with this PR? |
Yea, I currently don't have the bandwidth to take this. I'll post it internally and see if we get anyone to work on it. |
Hi @luacmartins let us know if you have any updates on it |
No updates yet. Nobody volunteered 😞 |
@hoangzinh You can proceed with the review now 👍 |
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.
Code is looking good. Only issue that I noticed is that the report preview doesn't update the correct total after submitting an expense
Screen.Recording.2025-01-27.at.12.20.54.PM.mov
@FitseTLT let's address my comment above. @hoangzinh did you notice any other issues while testing the PR? |
@luacmartins The behaviour is a bit different in my case. The submit it action properly sets the expense report's total but the openReport API called after 2025-01-29.01-41-27.mp4 |
It's weird that I'm unable to reproduce above issue Screen.Recording.2025-01-29.at.20.48.14.mov |
@luacmartins So far so good to me except for the warning log above |
But I fixed that @hoangzinh Is it still reproducible? |
Ah nope, It's no longer reproducible |
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.
@luacmartins The behaviour is a bit different in my case. The submit it action properly sets the expense report's total but the openReport API called after AddTrackedExpenseToPolicy API resets back to some other value and the report preview changes after a while as you can see in my vid but from my investigation the same behaviour doesn't occur for submit to a dm and OpenReport called after ConvertTrackedExpense API in that case sets the iou report to the correct total
Don't you think the openReport API's behavior is buggy?
I think the issue is that we're calling addTrackedExpenseToPolicy
without the reimbursable
param set to true. The API default is false, so the new request is always treated as non-reimbursable. When I updated your branch to send the param, things seem to work correctly
@luacmartins I passed reimbursable param as true for addTrackedExpenseToPolicy 👍 |
Nice! @hoangzinh let's give this another round of review |
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
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
🚀 Deployed to staging by https://github.com/luacmartins in version: 9.0.94-0 🚀
|
🚀 Deployed to production by https://github.com/AndrewGable in version: 9.0.94-25 🚀
|
🚀 Deployed to production by https://github.com/AndrewGable in version: 9.0.94-25 🚀
|
Details
Fixed Issues
$ #51552
PROPOSAL: #51552 (comment)
Tests
Offline tests
Same as above
QA Steps
Same as above
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(themeColors.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
Android: mWeb Chrome
2025-01-22.01-30-57.mp4
iOS: Native
2025-01-21.23-47-42.mp4
iOS: mWeb Safari
2025-01-21.23-49-49.mp4
MacOS: Chrome / Safari
2025-01-22.00-20-40.mp4
MacOS: Desktop
2025-01-22.00-19-42.mp4