Update MoneyRequestView for time expenses#77748
Conversation
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.
|
|
@ShridharGoel 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] |
| amountDescription += ` ${CONST.DOT_SEPARATOR} ${translate('iou.time')}`; | ||
| } else if (isDistanceRequest) { | ||
| amountDescription += ` ${CONST.DOT_SEPARATOR} ${translate('common.distance')}`; | ||
| } else if (isPerDiemRequest) { | ||
| amountDescription += ` ${CONST.DOT_SEPARATOR} ${translate('common.perDiem')}`; | ||
| } else if (isCardTransaction) { | ||
| amountDescription += ` ${CONST.DOT_SEPARATOR} ${translate('iou.card')}`; | ||
| } else { | ||
| amountDescription += ` ${CONST.DOT_SEPARATOR} ${translate('iou.cash')}`; |
There was a problem hiding this comment.
Since ${CONST.DOT_SEPARATOR} is common in all, can we append it before the if-else ?
There was a problem hiding this comment.
done + I refactored this part a little bit
| } else if (isPerDiemRequest) { | ||
| amountDescription += translate('common.perDiem'); | ||
| } else if (isCardTransaction) { | ||
| amountDescription += translate('iou.card'); |
There was a problem hiding this comment.
This card one looks newly added, is that expected?
There was a problem hiding this comment.
yeah, from what I've seen on Figma we want to display all expense types in the amountDescription, the way we do for "cash" currently. cc: @dannymcclain
There was a problem hiding this comment.
I mean, I personally think that makes the most sense. Why would we label one type but no others? cc @dubielzyk-expensify @Expensify/product
There was a problem hiding this comment.
Yeah I'm a big fan of the consistency of label all types
There was a problem hiding this comment.
We use the space of the dot separator to house original currency and converted currency and stuff like that with card transactions, and I think there will be a necessity for cash expenses as well on the converted amount front (CC: @JmillsExpensify).
I'm actually not sure why we still have the Cash dot separator here to be honest. I think it's a bit of a hangover from a very early world of not having the Type column and table. We don't seem to be showing Distance or Per diem on prod either.
There was a problem hiding this comment.
I love removing this for simplicity
|
In |
I believe this component is used for when we're creating new expenses, not viewing existing ones. I will handle that part in this PR: #78137 |
Reviewer Checklist
Screenshots/Videos |
ooh, good catch. it should be a quick fix, I will handle it in this PR |
done! |
|
@mhawryluk Should we update the icon in the expense report?
|
|
|
||
| function isTaxTrackingEnabled(isPolicyExpenseChat: boolean, policy: OnyxEntry<Policy>, isDistanceRequest: boolean, isPerDiemRequest = false): boolean { | ||
| if (isPerDiemRequest) { | ||
| function isTaxTrackingEnabled(isPolicyExpenseChat: boolean, policy: OnyxEntry<Policy>, isDistanceRequest: boolean, isPerDiemRequest = false, isTimeRequest = false): boolean { |
There was a problem hiding this comment.
NAB (Don't block merge): I suggest passing requestType instead of a boolean parameter, so we can handle each request type explicitly (e.g. isDistanceRequest, isPerDiemRequest, isTimeRequest)
There was a problem hiding this comment.
This will make it easier to scale and maintain
There was a problem hiding this comment.
that would be great, but I think in most places where the isTaxTrackingEnabled function is used we have type information stored as a series of boolean values. so we would need to convert those to a string requestType value. changing the whole repository to not pass around the boolean values would be quite complicated. we could create a util function that would accept the boolean values and spit out a single string, but then we would have 2 patterns, but maybe that's okay? or at the very least I could make isTaxTrackingEnabled accept an object, so that the parameters are named when passed. what do you think?
There was a problem hiding this comment.
oh, I forgot we have getRequestType function. I think we can use it here
There was a problem hiding this comment.
nevermind, we can't use it yet, getRequestType is modified to include time requests in the expense creation PR, because that's when we add the new request type
There was a problem hiding this comment.
we don't use type for distance at the moment, only time and per diem I believe are really using it right now. So we might need to clean up a few things for this suggestion
| } else if (isTimeRequest(transaction)) { | ||
| previewHeaderText = [{translationPath: 'iou.time'}]; |
There was a problem hiding this comment.
NAB: Suggest using a mapping instead of checking each case one by one
There was a problem hiding this comment.
not sure how to achieve this. we don't have a requestType variable that we could use as a key for the mapping, instead we determine type by running the is<Type>Request functions. do you mean we should first determine requestType by running all the function and then set previewHeaderText using a map?
| } else if (isCardTransaction) { | ||
| amountDescription += translate('iou.card'); |
There was a problem hiding this comment.
@mhawryluk The card transaction is handled below. I don’t think this change aligns with the design doc or could you please share the design document related to this change?
There was a problem hiding this comment.
The same to per diem and distance requests. This change seems unrelated to our project. To avoid regressions after merging this PR, I’d like to confirm that this change is intentional
There was a problem hiding this comment.
yeah, I took this from a discussion on Figma, but it looks like we are going to remove this part of the label after all. waiting for the confirmation on this one.
cc: @dannymcclain it's this label:
There was a problem hiding this comment.
Yeah I believe that's what @trjExpensify was saying: we don't need this dot separator type indicator on these Amount push inputs. Those would only show on the preview. Tom can confirm!
|
@mhawryluk Kindly bump |
it should be updated by this PR: #77753 |
|
✋ 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/grgia in version: 9.2.96-1 🚀
|
|
🚀 Deployed to production by https://github.com/arosiclair in version: 9.2.96-6 🚀
|
|
🚀 Deployed to production by https://github.com/arosiclair in version: 9.2.96-6 🚀
|
|
🚀 Deployed to production by https://github.com/arosiclair in version: 9.2.96-6 🚀
|
Time expenses do not support taxes (established in Expensify#77748). This fix hides TAX_RATE and TAX_AMOUNT columns for time expense transactions in the Reports > Expenses view. Fixes Expensify#80208
| const shouldShowAttendees = shouldShowAttendeesTransactionUtils(iouType, policy); | ||
|
|
||
| const shouldShowTax = isTaxTrackingEnabled(isPolicyExpenseChat, policy, isDistanceRequest, isPerDiemRequest); | ||
| const shouldShowTax = isTaxTrackingEnabled(isPolicyExpenseChat, policy, isDistanceRequest, isPerDiemRequest, isTimeRequest); |
There was a problem hiding this comment.
For time expenses, tax and tax rate should also be empty in search columns. We fixed this in #80208










Explanation of Change
Disables showing the tax information when viewing existing time expenses (created in old dot). Displays showing expense type next to the "Amount" row label.
Fixed Issues
$ #77379
PROPOSAL: N/A
Tests
Offline tests
N/A
QA Steps
Same as tests.
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
Nagranie.z.ekranu.2025-12-22.o.10.37.29.mov
Android: mWeb Chrome
Nagranie.z.ekranu.2025-12-22.o.10.35.07.mov
iOS: Native
Simulator.Screen.Recording.-.iPhone.16.Pro.Max.-.2025-12-22.at.10.32.39.mp4
iOS: mWeb Safari
Simulator.Screen.Recording.-.iPhone.16.Pro.Max.-.2025-12-22.at.10.29.39.mp4
MacOS: Chrome / Safari