-
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 distance amount changes after submitting track distance expense with description #55063
Fix distance amount changes after submitting track distance expense with description #55063
Conversation
@dukenv0307 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] |
@narefyev91 I can't submit the track distance expense Screen.Recording.2025-01-13.at.11.53.08.mov |
Did you face this issue? |
# Conflicts: # src/libs/actions/IOU.ts
# Conflicts: # src/libs/actions/IOU.ts
Yeah - seen the same problem. Will ask BE engineer what's happened. Probably we do not need to pass |
@dukenv0307 we get clarification here #51417 (comment). I made changes and double checked that it's working now |
# Conflicts: # src/libs/API/parameters/ShareTrackedExpenseParams.ts # src/libs/actions/IOU.ts
on it now |
@narefyev91 Bug: The distance and rate change after submitting track expense Screen.Recording.2025-01-22.at.15.18.06.mov |
# Conflicts: # src/libs/actions/IOU.ts
Yeah seeing the same issue.. Generally my workspace does not have enabled Distance Rate feature That's why i think after submit categorised expense - BE automatically switch to default one. @rlinoz could you please check here as well. Is it intentional? |
@rlinoz friendly bump on this one |
Sorry, I believe it is intentional, since the rate of the policy is not the same as the original one, I will check out the code either today or tomorrow |
# Conflicts: # src/libs/API/parameters/CategorizeTrackedExpenseParams.ts # src/libs/actions/IOU.ts
So yeah, because we are sending the milage rate as undefined, as we should, the backend will apply the default rate for the policy. So I think we could update the Categorize panel to use the default rate for the policy already, so the user will already know how this will look like. |
Generally i think we are doing everything right here - if user does not have correct rate - BE will just pick default one. |
Im good with that, and we can always improve if needed later |
@dukenv0307 could you please continue review |
@narefyev91 I'm on the holiday, I'll complete the review in 1-2 days. Sorry for this delay |
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2025-01-29.at.20.04.19.movAndroid: mWeb ChromeScreen.Recording.2025-01-29.at.19.57.20.moviOS: NativeScreen.Recording.2025-01-29.at.19.59.43.moviOS: mWeb SafariScreen.Recording.2025-01-29.at.19.56.04.movMacOS: Chrome / SafariScreen.Recording.2025-01-29.at.19.52.32.movMacOS: DesktopScreen.Recording.2025-01-29.at.20.07.12.mov |
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
Will finish review tomorrow |
@dukenv0307 @rlinoz i create a new PR #56146 - to resolve issue with main (which we get yesterday) - i will close this one - and let's continue with new one |
Explanation of Change
Implement required changes from BE request: we need to pass both waypoints and customUnitRateID for CategorizeTrackedExpense and ShareTrackedExpense API calls
Fixed Issues
$ #51417
PROPOSAL: #51417 (comment)
Tests
Offline tests
Nothing changed in offline mode
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodSTYLE.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 and/or tagged@Expensify/design
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.mov
Android: mWeb Chrome
android-web.mov
iOS: Native
ios.mov
iOS: mWeb Safari
ios-web.mov
MacOS: Chrome / Safari
web.mov
MacOS: Desktop
desktop.mov