Allow creating time expenses from global menu #79780
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 |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
…expense-global-create
blazejkustra
left a comment
blazejkustra
left a comment
There was a problem hiding this comment.
One NAB, other than that LGTM 🚀
melvin-bot
bot
requested review from
DylanDylann and
heyjennahay
and removed request for
a team
|
@DylanDylann 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] |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f35e93e3dc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f35e93e3dc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (!transactionID) { | ||
| return; | ||
| } | ||
| Navigation.navigate(ROUTES.MONEY_REQUEST_STEP_TIME_RATE.getRoute(action, iouType, transactionID, reportID, reportActionID)); |
There was a problem hiding this comment.
Do we need to update this one as well? @mhawryluk
There was a problem hiding this comment.
no, I separated step hours into hours and hours_edit, because it is now appearing in two scenarios: accessed when clicking on the hours row on the confirmation page (hours_edit) and after selecting a workspace in the time tab on the "create expense" start page (hours) and we use the same component, but there are a few places we need to differentiate the two scenarios. time_rate step is only used in one scenario so we don't have to update it. would you like me to rename it to time_rate_edit though to match hours_edit?
| if (explicitPolicyID) { | ||
| const policyExpenseChatReportID = setMoneyRequestParticipantAsPolicyExpenseChat({ | ||
| transactionID, | ||
| policyID: explicitPolicyID, | ||
| currentUserAccountID: accountID, | ||
| isDraft: isTransactionDraft, | ||
| participantsAutoAssigned: true, | ||
| }); | ||
| return Navigation.setNavigationActionToMicrotaskQueue(() => | ||
| Navigation.navigate(ROUTES.MONEY_REQUEST_STEP_CONFIRMATION.getRoute(CONST.IOU.ACTION.CREATE, iouType, transactionID, policyExpenseChatReportID ?? reportID)), | ||
| ); | ||
| } | ||
| setMoneyRequestParticipantsFromReport(transactionID, report, accountID); | ||
| } |
There was a problem hiding this comment.
Why setMoneyRequestParticipantAsPolicyExpenseChat instead of setMoneyRequestParticipantsFromReport() but with policyExpenseChatID/.
There was a problem hiding this comment.
good point, I will change this. it's just that apart from participants the new function also sets reportID, but I can call 2 actions instead.
| type IOURequestStepPerDiemWorkspaceProps = PlatformStackScreenProps<MoneyRequestNavigatorParamList, typeof SCREENS.MONEY_REQUEST.CREATE>; | ||
|
|
||
| type IOURequestStepPerDiemWorkspaceProps = WithWritableReportOrNotFoundProps<typeof SCREENS.MONEY_REQUEST.CREATE> & WithFullTransactionOrNotFoundProps<typeof SCREENS.MONEY_REQUEST.CREATE>; | ||
| function IOURequestStepPerDiemWorkspace({route, navigation}: IOURequestStepPerDiemWorkspaceProps) { |
There was a problem hiding this comment.
Do we need to add any extra QA steps as this impacts per diem?
There was a problem hiding this comment.
provided I didn't make any mistakes this should be just a code reorganization without change to how anything works, but we should definitely check, I will add a test case
There was a problem hiding this comment.
That's a great idea for creating a common workspace list component. However, next time, I'd recommend doing this in a separate PR for easier testing. This approach also ensures that any regressions won't impact the progress of our feature.
There was a problem hiding this comment.
sure, I'll keep that in mind for next time
| } | ||
|
|
||
| setMoneyRequestTimeRate(transactionID, rate, isTransactionDraft); | ||
| setMoneyRequestParticipantsFromReport(transactionID, report, accountID).then(() => |
There was a problem hiding this comment.
should we remove this .then?
There was a problem hiding this comment.
that's a very good question. I first implemented it using .then cause this is how it's done in the manual step. but then @blazejkustra noticed that this doesn't follow our best practices, as we shouldn't return promises from actions and then wait for their completion. however this approach introduced a bug I noticed on iOS (although for setMoneyRequestParticipantAsPolicyExpenseChat, not setMoneyRequestParticipantsFromReport), navigation was incorrect when Onyx update hadn't completed yet and I fixed it (hopefully) by using Navigation.setNavigationActionToMicrotaskQueue which should ensure the navigation happens after onyx updates
| if (isTimeRequest) { | ||
| return Navigation.goBack(ROUTES.MONEY_REQUEST_CREATE_TAB_TIME.getRoute(action, iouType, initialTransactionID, reportID)); | ||
| } |
There was a problem hiding this comment.
this block was unnecessary, the function should correctly handle navigation without it. I added it because I noticed the navigation wasn't correct and I was inspired by perDiem, but it turned out this block introduced some bugs and the thing that was missing was adding support for "time" type in navigateToStartMoneyRequestStep
this change was already merged here: https://github.com/Expensify/App/pull/79673/files
src/libs/actions/IOU/index.ts
Outdated
| if (!policyExpenseChatReportID) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Do we catch this silent error?
There was a problem hiding this comment.
kinda 😅 when it is undefined we instead use reportID from route, which is probably not correct. I can make it so that we don't navigate and log an error. though I don't think this is likely to happen. we only allow to select a workspace that has a chat, so this reportID should be defined
|
Mostly questions and clarification ^^^^ |
|
🚧 @grgia has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
|
🚧 @grgia has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
src/ROUTES.ts
Outdated
| getRoute: (action: IOUAction, iouType: IOUType, transactionID: string | undefined, reportID: string | undefined, reportActionID?: string) => | ||
| `${action as string}/${iouType as string}/hours/${transactionID}/${reportID}${reportActionID ? `/${reportActionID}` : ''}` as const, | ||
| }, | ||
| MONEY_REQUEST_STEP_HOURS_EDIT: { |
There was a problem hiding this comment.
Could we use MONEY_REQUEST_STEP_HOURS for the edit flow, like the others? The action parameter already distinguishes between edit and create flows
There was a problem hiding this comment.
in both cases the action parameter is "create", so the urls would be the same. I would need to add another parameter to distinguish between the two. should I?
There was a problem hiding this comment.
Ahh I understand your problem.
There was a problem hiding this comment.
@mhawryluk We also get this problem on other expense create flow. And we resolved it by checking the route name. Could you check this conversation and see If we can apply them here?
There was a problem hiding this comment.
the step after selecting a workspace would also be IOURequestStepHours and action "create". it's this one:
Nagranie.z.ekranu.2026-01-21.o.13.37.10.mov
There was a problem hiding this comment.
@mhawryluk Ahh got it. Could we use transaction.amount or transaction.units.count to distinguish them? Or if impossible, I suggest creating a new parameter for the current route instead of creating a new route
There was a problem hiding this comment.
I think basing it on transaction properties will not work, because we can go back from the confirmation page and then the transaction is going to be filled out. I will check this, and if that's the case then I will add a new parameter
There was a problem hiding this comment.
Thank you, @mhawryluk. Please ping me when your update is ready.
| getRoute: ( | ||
| action: IOUAction, | ||
| iouType: IOUType, | ||
| origin: ValueOf<typeof CONST.IOU.HOURS_STEP_ORIGIN>, |
There was a problem hiding this comment.
@mhawryluk After creating the time expense, in the edit flow, is this undefined?
There was a problem hiding this comment.
we don't have edit flow for time expense hours after the time expense is created
There was a problem hiding this comment.
from the confirmation page it is "confirm"
There was a problem hiding this comment.
@mhawryluk Would you mind if I requested the change one more time? 😄
There was a problem hiding this comment.
@mhawryluk Sorry for the back and forth, but adding a new value to the action param looks cleaner than creating a new param. What do you think?
There was a problem hiding this comment.
hmm, not sure. IOUAction is used quite extensively and I worry it would be difficult to update all necessary places. I'll see if it's doable
There was a problem hiding this comment.
If you're concerned about changing IOUAction, we could create a new type for time expense. That said, I don't think it would affect other flows since they won't use this new value
There was a problem hiding this comment.
I don't know if it's a good idea to mess with action at all. it looks like a core part in each of the money request steps for all types of expenses. we would create a new pattern here. tbh I think I like the first version with creating 2 routes the best, but the one with a new parameter is also okay for me
| const isEditingConfirmation = (params as MoneyRequestNavigatorParamList[typeof SCREENS.MONEY_REQUEST.STEP_HOURS])?.origin === CONST.IOU.HOURS_STEP_ORIGIN.CONFIRM; | ||
| const isEmbeddedInStartPage = routeName === SCREENS.MONEY_REQUEST.CREATE; | ||
| const policyID = explicitPolicyID ?? report?.policyID; | ||
| const isTransactionDraft = shouldUseTransactionDraft(action); |
There was a problem hiding this comment.
If we can't edit the time expense, does that mean isTransactionDraft will always be true?
There was a problem hiding this comment.
yes, I think isTransactionDraft is always true in this step, but I thought I would use the util anyways to make sure it's correct. but I can remove it if you want
| function IOURequestStepHours({report, route: {params, name: routeName}, transaction, explicitPolicyID}: IOURequestStepHoursProps) { | ||
| const {iouType, reportID, transactionID = '-1', action, reportActionID} = params; | ||
| const isEditingConfirmation = (params as MoneyRequestNavigatorParamList[typeof SCREENS.MONEY_REQUEST.STEP_HOURS])?.origin === CONST.IOU.HOURS_STEP_ORIGIN.CONFIRM; | ||
| const isEmbeddedInStartPage = routeName === SCREENS.MONEY_REQUEST.CREATE; |
There was a problem hiding this comment.
This check looks outdated, now
There was a problem hiding this comment.
it's not, this is for when the component is used directly in the "time" tab in the create expense RHP. so when starting to create an expense in a workspace chat or from global create, but when there is just one workspace
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
| const {iouType, reportID, transactionID = '-1', action, reportActionID} = params; | ||
| const isEditingConfirmation = (params as MoneyRequestNavigatorParamList[typeof SCREENS.MONEY_REQUEST.STEP_HOURS])?.origin === CONST.IOU.HOURS_STEP_ORIGIN.CONFIRM; |
There was a problem hiding this comment.
| const {iouType, reportID, transactionID = '-1', action, reportActionID} = params; | |
| const isEditingConfirmation = (params as MoneyRequestNavigatorParamList[typeof SCREENS.MONEY_REQUEST.STEP_HOURS])?.origin === CONST.IOU.HOURS_STEP_ORIGIN.CONFIRM; | |
| const {iouType, reportID, transactionID = '-1', action, reportActionID, origin} = params; | |
| const isEditingConfirmation = origin === CONST.IOU.HOURS_STEP_ORIGIN.CONFIRM; |
| if (isEmbeddedInStartPage) { | ||
| if (explicitPolicyID) { | ||
| const policyExpenseChat = getPolicyExpenseChat(accountID, policyID); | ||
| if (!policyExpenseChat) { | ||
| console.error(`Couldn't find policy expense chat for policyID: ${policyID}`); | ||
| return; | ||
| } | ||
|
|
||
| setTransactionReport(transactionID, {reportID: policyExpenseChat.reportID}, isTransactionDraft); | ||
| setMoneyRequestParticipantsFromReport(transactionID, policyExpenseChat, accountID); | ||
|
|
||
| return Navigation.setNavigationActionToMicrotaskQueue(() => | ||
| Navigation.navigate(ROUTES.MONEY_REQUEST_STEP_CONFIRMATION.getRoute(CONST.IOU.ACTION.CREATE, iouType, transactionID, policyExpenseChat.reportID)), | ||
| ); | ||
| } | ||
| setMoneyRequestParticipantsFromReport(transactionID, report, accountID); | ||
| } |
There was a problem hiding this comment.
Is there a reason we're setting report and participant here? I would expect this to happen when the user selects the workspace.
4 participants
Explanation of Change
Enables the "time" tab in "Create expense" RHP accessed through global menu, when there is at least one workspace with time tracking enabled. If there are multiple, a list of workspaces to select is displayed.
Fixed Issues
$ #77684
PROPOSAL: N/A
Tests
Scenario 0: No beta enabled.
Scenario 1: No workspaces with time tracking enabled.
Scenario 2: 1 workspace with time tracking enabled.
Scenario 3: Multiple workspaces with time tracking enabled.
Extra 1: Ensure no regression when creating from the workspace chat/report.
Extra 2: Ensure no regression when creating Per Diem expenses via global menu.
Offline tests
Same as tests but offline.
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.2026-01-20.o.13.15.45.mov
Android: mWeb Chrome
Nagranie.z.ekranu.2026-01-20.o.13.09.50.mov
iOS: Native
Simulator.Screen.Recording.-.iPhone.16.Pro.Max.-.2026-01-20.at.12.45.01.mp4
iOS: mWeb Safari
Simulator.Screen.Recording.-.iPhone.16.Pro.Max.-.2026-01-20.at.13.08.22.mp4
MacOS: Chrome / Safari
Nagranie.z.ekranu.2026-01-20.o.13.20.05.mov