Conversation
Co-authored-by: Cong Pham <suneox@users.noreply.github.com>
|
I created this PR from an upstream branch because I don't have push access to your fork. If you want to take ownership of this branch and push updates yourself, run: Then close this PR and open a new one from your fork. |
|
The What's failing: three tests in
All three assert on Already tracked and already being fixed: #100987 ( Nothing to do here. Once either fix PR merges, merge EvidenceThe same three assertions fail identically on unrelated open PRs:
The other red check, view run · no recording available |
|
🤖 Android test steps verification results: Only the two single-account steps could run. The steps that prove this PR's actual fix (2, 3, 4) all require a second signed-in user, and step 6 needs a domain admin plus a separate employee with a managed-card transaction — this Android session provides exactly one authenticated account, and minting or signing into another mid-run is a documented hard blocker. The read-only-for-non-owner behavior this PR changes was not verified on Android. No console errors were observed during the steps that ran. Steps 2–4 and 6 need a manual QA pass with two provisioned accounts, or a Melvin run in an environment that can drive two concurrent authenticated sessions. view run · no recording available |
|
🤖 Web test steps verification results: ❌ fail Setup for step 1 was interrupted when a "Referral" promo panel appeared over the in-progress Create Expense form and its control triggered a full-page navigation off the app to the public expensify.com marketing site. That is an unrecoverable session-killer, so driving stopped before any PR-specific assertion could be checked. Separately, steps 3, 4 and 6 could not have run regardless — they need a second signed-in user (and for step 6, a domain admin plus an employee's managed-card transaction), and this web session provides exactly one account. No console errors were captured before the session ended. Nothing about this PR was verified on web. The referral panel navigating off-app looks like a pre-existing hazard unrelated to this PR and is worth a separate look, since it can block verification of any PR. To cover these steps, this needs a manual QA pass with two provisioned accounts. view run · no recording available |
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |





Explanation of Change
When user B opens a track expense that user A created in A's self-DM, every coding field (Description, Category, Tag, Billable, Report) renders as editable for B, and tapping Report → Create report fails with "Unexpected error creating this chat. Please try again later."
MoneyRequestViewswaps inpolicyForMovingExpensesfor unreported expenses — that is the workspace the viewer would move the expense to, not the workspace the expense belongs to. It then passes that policy straight intocanEditMoneyRequest, whereisAdminwas computed asreportPolicy?.role === ADMINwith no report-type guard. B admins their own workspace, soisAdminwastrueand the function returned early, skipping the requestor check entirely.canEditFieldOfMoneyRequestgates oncanEditMoneyRequestfirst, so every field inherited the same wrong answer. The error message is downstream: because the Report row was interactive, B could reachCreateAppReport, which sendsownerEmail= A withpolicyID= B's workspace, and A is not a member of it.This qualifies
isAdminandisManagerwithisExpenseReport(moneyRequestReport), matching whatcanCurrentUserEditExpensealready does. A self-DM is not an expense report, so B now falls through to the owner check and every field is read-only. The guard also closes theisManagervariant of the same hole: a self-DM has nomanagerID, so an unresolveddeprecatedCurrentUserAccountIDwould otherwise match it.Two behaviors are deliberately left alone:
isRequestorat the end of the function. The new unit test asserts both directions.|| isSelectedReportUnreportedinIOURequestEditReportCommonstays. The original proposal suggested dropping it; per review that would turn the Report step into/not-foundfor a domain admin reporting an employee's unreported managed-card transaction. Those transactions early-returntruebefore the policy is read, so that flow is unchanged by this PR.The alternative of not passing
policyForMovingExpensesinto the permission helpers at all was considered and rejected in review: it swaps the policy behind every permission read inMoneyRequestViewand carries regression risk on the wrong-workspace resolution.Fixed Issues
$ #98099
PROPOSAL: #98099 (comment)
Tests
// TODO: The human co-author must fill out the tests they ran before marking this PR as "ready for review".
// Please describe what tests you performed that validate the change worked.
Automated checks Melvin ran
npm run typechecknpm run lint-changednpm run spell-changednpm test -- tests/unit/ReportUtilsTest.tsnpm test -- tests/unit/canEditFieldOfMoneyRequestTest.ts tests/unit/inlineEditing/TransactionInlineEdit.test.ts tests/unit/ReportSecondaryActionUtilsTest.ts tests/unit/hooks/useSelectedTransactionsActions.test.ts tests/actions/IOUTest/BulkEditTest.tsnpm test -- tests/ui/MoneyRequestViewTest.tsx tests/ui/MoneyRequestViewReceiptTest.tsx tests/ui/MoneyReportContentCreatedTest.tsx tests/ui/ReportActionComposeTest.tsxThe new unit test was confirmed to fail against
main(source change stashed) and pass with the fix, so it genuinely covers the regression.Not run, and why: the full
npm testsuite and the Storybook smoke test were skipped for runtime; the suites above are every test file that referencescanEditMoneyRequest,canEditFieldOfMoneyRequest,canCurrentUserEditExpense, orMoneyRequestView.npm run prettierno longer exists as a script in this repo — formatting is enforced through ESLint, which passed. Manual browser verification did not happen: the automated browser session went blank mid-run and could not be restarted within the run, so the owner-still-can-edit path has unit coverage but no screenshot.Offline tests
QA Steps
// TODO: The human co-author must fill out the QA steps before marking this PR as "ready for review".
// These must be filled out, or the issue title must include "[No QA]."
//
// Suggested starting point, from the issue repro (needs 2 users on 2 devices):
// 1. As user A, open your self-DM and create a manual track expense.
// 2. In that expense, mention user B and choose "Invite to chat only".
// 3. As user B, open user A's track expense.
// 4. Verify Description, Category, Tag, Billable and Report are all read-only for user B, and that
// "Unexpected error creating this chat. Please try again later." never appears.
// 5. As user A, open the same expense and verify those fields are still editable for you.
// 6. Regression: as a domain admin, open an employee's unreported managed-card transaction, tap Report,
// and verify the report-selection step opens instead of /not-found.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, 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.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