Filter time expenses from workspaces with disabled time tracking#84655
Filter time expenses from workspaces with disabled time tracking#84655justinpersaud merged 5 commits intomainfrom
Conversation
Mirrors the existing per diem filtering pattern to prevent time expenses from being submitted to workspaces that have time tracking disabled. This addresses both the report selection flow and the recipient/participant selection flow. Co-authored-by: Krishna <Krishna2323@users.noreply.github.com>
|
I created this PR from an upstream branch since I don't have push access to your fork. To take ownership of this branch and be able to push updates, run: Then you can close this PR and open a new one from your fork. |
Co-authored-by: Krishna <Krishna2323@users.noreply.github.com>
|
Fixed the failing Prettier check: ran Prettier on |
|
The failing check Analysis: The Rock remote build itself completed successfully and downloaded the cached APK. The failure occurs in the subsequent CI step that tries to locate the APK at Evidence:
Re-running the failed check should resolve this. |
|
The failing checks are unrelated to this PR's changes. ESLint check: All 339 warnings are in files not modified by this PR — build (Debug, true): The Android build failed because the Evidence: This PR only modifies files in |
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.
|
|
@codex review |
![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: af4ea06623
ℹ️ 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".
| const isAndroidNative = getPlatform() === CONST.PLATFORM.ANDROID; | ||
| const isMobileSafari = isMobileSafariBrowser(); | ||
| const isPerDiem = isPerDiemRequest(initialTransaction); | ||
| const isTime = isTimeRequestUtil(initialTransaction); |
There was a problem hiding this comment.
The new isTimeRequest flag is derived from initialTransaction, but this screen is still usable before that object is loaded (the handler explicitly falls back to initialTransactionID when transactions.length === 0). In that loading window isTime is false, so the participant list is built without the new time-request restrictions, which lets users on slow hydration paths pick personal contacts or workspaces with disabled time tracking before the filter turns on.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I looked into this. The concern about a hydration race window is not a new issue introduced by this PR — it's a pre-existing architectural characteristic that applies equally to all transaction-derived flags.
Key findings:
-
Identical pattern to
isPerDiemRequest: The newisTimeflag is derived frominitialTransactionin exactly the same way as the existingisPerDiemandisCorporateCardflags (IOURequestStepParticipants.tsx:127-130). All three returnfalsewheninitialTransactionisundefined. -
Pre-existing HOC behavior:
withFullTransactionOrNotFoundwraps this component withshouldShowLoadingIndicator = false(the default), meaning it renders immediately during Onyx loading. This affects ALL step screens using this HOC and ALL transaction-derived values — not justisTimeRequest. -
Practically unreachable: The transaction draft is written to Onyx via
Onyx.set()ininitMoneyRequest(withiouRequestType: TIME) before the first step of the expense flow. The user must navigate through the time entry screen(s) before reaching the participants screen, giving Onyx ample time to hydrate. -
Consistent with the codebase: The PR adds
isTimeRequestalongside every existingisPerDiemRequestcheck — this is a straightforward "mirror the existing pattern" implementation.
If the hydration window were a real concern, it would need to be addressed as a separate issue for isPerDiemRequest, isCorporateCardTransaction, iouRequestType, and isWorkspacesOnly equally — not as a blocker for this PR specifically.
|
The failing checks are unrelated to this PR's changes. 1. ESLint check — The full ESLint check is failing due to pre-existing
None of these files are in this PR's diff. The 2. build (Debug, true) — This is a CI infrastructure issue. The cached build was successfully downloaded from S3, but the subsequent step failed looking for the APK at a local path ( 3. checklist — This is the PR Author/Reviewer Checklist check, which is not a code issue. No code fix is needed from this PR's side. |
|
The failing checks are unrelated to this PR's changes. 1. ESLint check — All 339 warnings are in files not modified by this PR:
The Changed files ESLint check (which only lints files in this PR) passed successfully, confirming these are pre-existing warnings. 2. build (Debug, true) — CI infrastructure issue. The build downloaded a cached APK from S3 successfully, but the subsequent step failed with: This is a build pipeline path resolution issue, not related to any code changes in this PR. Evidence: This PR only modifies these files, none of which overlap with the failures:
|
|
I have failed to help you. I let you down and for that I am sorry. Please try again later. This may be due to a Claude outage (check here). Failure details: https://github.com/Expensify/Expensify/actions/runs/22996647905 |
|
@MelvinBot resolve conflicts and mark it as ready. |
…nsesByTimeTrackingEnabled Co-authored-by: Krishna <Krishna2323@users.noreply.github.com> # Conflicts: # src/pages/iou/request/step/IOURequestEditReportCommon.tsx
melvin-bot
bot
requested review from
Krishna2323
and removed request for
a team
|
@Krishna2323 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] |
melvin-bot
bot
requested review from
heyjennahay
and removed request for
a team
|
Resolved merge conflicts and marked the PR as ready for review. Conflict resolution: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7419245089
ℹ️ 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 (isTimeRequest && !isTimeTrackingEnabled(policy)) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
This new filter only removes existing reports from the picker, but it does not guard the Create report path, which still gets its destination from usePolicyForMovingExpenses (that hook only validates per-diem constraints). In a time request flow where no valid report is selected yet (for example, moving an unreported/self-DM time expense), users can still create a new report in a workspace with time tracking disabled, so the restriction added here can be bypassed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This also seems pre-existing issue, @MelvinBot could you please confirm?
There was a problem hiding this comment.
This one is not a pre-existing issue — it's a genuine gap in the PR, unlike the previous hydration concern.
Key difference from per diem: The usePolicyForMovingExpenses hook (src/hooks/usePolicyForMovingExpenses.ts:23-30) already guards the "Create report" path for per diem by checking canSubmitPerDiemExpenseFromWorkspace(policy). For time requests, no equivalent guard exists:
// Current check in isPolicyValidForMovingExpenses:
(!isPerDiemRequest || canSubmitPerDiemExpenseFromWorkspace(policy))
// No equivalent: (!isTimeRequest || isTimeTrackingEnabled(policy))The hook is called at IOURequestStepReport.tsx:83 with only isPerDiemRequest(transaction) — no time request flag is passed. So for a time expense, the "Create report" button could point to a workspace with time tracking disabled.
To make this fully equivalent to per diem, usePolicyForMovingExpenses would need:
- An
isTimeRequestparameter - An
(!isTimeRequest || isTimeTrackingEnabled(policy))check inisPolicyValidForMovingExpenses - The caller at
IOURequestStepReport.tsx:83would need to pass the time request flag
There was a problem hiding this comment.
Fixed in 5cbf8e3. Added isTimeRequest parameter to usePolicyForMovingExpenses so the "Create report" path now filters out workspaces with time tracking disabled, mirroring the existing per diem guard.
Changes:
usePolicyForMovingExpensesnow acceptsisTimeRequest?: booleanand checks(!isTimeRequest || isTimeTrackingEnabled(policy))inisPolicyValidForMovingExpensesand the active policy check- Updated callers in
IOURequestStepReport,IOURequestEditReportCommon, andSearchTransactionsChangeReportto pass the new parameter
Add isTimeRequest parameter to usePolicyForMovingExpenses so the Create report button filters out workspaces that have time tracking disabled, mirroring the existing per diem guard. Co-authored-by: Krishna <Krishna2323@users.noreply.github.com>
melvin-bot
bot
requested review from
justinpersaud
and removed request for
a team
|
@justinpersaud 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] |
Co-authored-by: Krishna <Krishna2323@users.noreply.github.com>
|
Fixed the failing Prettier check: reformatted a long line in |
|
The failing Prettier check is unrelated to this PR's code changes — it's a transient GitHub Actions infrastructure failure. Analysis: The CI runner failed during the It retried 3 times and failed each time. Prettier never actually ran — the failure occurred before the code was even checked out. Evidence: The error is in the Next step: Please re-run the failed Prettier check. This should resolve on retry since it's a transient network issue. |
|
@MelvinBot prettier is still failing (fix if related) and could you also update the test steps? The flow has changed a bit:
Please verify this and update. |
|
Here's what I found and did: Prettier: The prettier fix was already committed ( Test steps updated. I verified the flow by tracing the code and updated the PR description with accurate test steps covering:
|
|
I analyzed the two failing checks on this PR. Both are unrelated to the PR's code changes. 1. Prettier check — Infrastructure failure (not code-related)The Prettier check failed during git checkout due to a GitHub Actions network error: This is a transient GitHub infrastructure issue. The check never reached the actual Prettier formatting step. 2. test (job 6) — Flaky test in
|
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid_native.mp4Android: mWeb Chromeandroid_chrome.mp4iOS: HybridAppios_native.mp4iOS: mWeb Safariios_safari.mp4MacOS: Chrome / Safariweb_chrome.mp4 |
There was a problem hiding this comment.
LGTM and works well! 🚀
@justinpersaud please re-run the tests, it should pass.
justinpersaud
deleted the
claude-filterTimeExpensesByTimeTrackingEnabled
branch
|
🚧 @justinpersaud 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! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
4 participants
Explanation of Change
Time expenses can currently be submitted to workspaces that have time tracking disabled. This mirrors the existing per diem filtering pattern to prevent time expenses from being submitted to workspaces with disabled time tracking.
Scenario 1 (Report Selection): When selecting a report for a time expense,
IOURequestEditReportCommonnow checksisTimeTrackingEnabled(policy)and filters out reports from workspaces where time tracking is disabled.Scenario 2 (Recipient/Participant Selection): When selecting a recipient/workspace for a time expense,
OptionsListUtilsandMoneyRequestParticipantsSelectornow filter out workspaces without time tracking enabled and exclude P2P recipients (since time tracking is a workspace-level feature).Fixed Issues
$ #81563
PROPOSAL: #81563 (comment)
Tests
Setup:
Scenario 1 — Multiple eligible workspaces (workspace selector):
Scenario 2 — Single eligible workspace (direct hours field):
Scenario 3 — Report selection filtering:
Scenario 4 — P2P excluded for time requests:
Offline tests
N/A — this change only affects client-side filtering of reports and participants. No network calls are involved in the filtering logic.
QA Steps
Scenario 1 — Multiple eligible workspaces:
Scenario 2 — Single eligible workspace:
Scenario 3 — Report filtering:
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand 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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari