Skip to content

Only let policy admins and managers edit expenses on expense reports - #101029

Draft
MelvinBot wants to merge 1 commit into
mainfrom
claude-restrictNonOwnerUnreportedExpenseEdit
Draft

MelvinBot wants to merge 1 commit into
mainfrom
claude-restrictNonOwnerUnreportedExpenseEdit

Conversation

@MelvinBot

Copy link
Copy Markdown
Contributor

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."

MoneyRequestView swaps in policyForMovingExpenses for 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 into canEditMoneyRequest, where isAdmin was computed as reportPolicy?.role === ADMIN with no report-type guard. B admins their own workspace, so isAdmin was true and the function returned early, skipping the requestor check entirely. canEditFieldOfMoneyRequest gates on canEditMoneyRequest first, so every field inherited the same wrong answer. The error message is downstream: because the Report row was interactive, B could reach CreateAppReport, which sends ownerEmail = A with policyID = B's workspace, and A is not a member of it.

This qualifies isAdmin and isManager with isExpenseReport(moneyRequestReport), matching what canCurrentUserEditExpense already 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 the isManager variant of the same hole: a self-DM has no managerID, so an unresolved deprecatedCurrentUserAccountID would otherwise match it.

Two behaviors are deliberately left alone:

  • Expense owners are unaffected. An owner's edit rights on an unreported expense never came from the policy — they come from isRequestor at the end of the function. The new unit test asserts both directions.
  • || isSelectedReportUnreported in IOURequestEditReportCommon stays. The original proposal suggested dropping it; per review that would turn the Report step into /not-found for a domain admin reporting an employee's unreported managed-card transaction. Those transactions early-return true before the policy is read, so that flow is unchanged by this PR.

The alternative of not passing policyForMovingExpenses into the permission helpers at all was considered and rejected in review: it swaps the policy behind every permission read in MoneyRequestView and 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
Check Result
npm run typecheck passed
npm run lint-changed passed
npm run spell-changed passed (0 issues)
npm test -- tests/unit/ReportUtilsTest.ts 1333 passed
npm 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.ts 310 passed
npm test -- tests/ui/MoneyRequestViewTest.tsx tests/ui/MoneyRequestViewReceiptTest.tsx tests/ui/MoneyReportContentCreatedTest.tsx tests/ui/ReportActionComposeTest.tsx 47 passed

The 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 test suite and the Storybook smoke test were skipped for runtime; the suites above are every test file that references canEditMoneyRequest, canEditFieldOfMoneyRequest, canCurrentUserEditExpense, or MoneyRequestView. npm run prettier no 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.

  • Verify that no errors appear in the JS console

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.

  • Verify that no errors appear in the JS console

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
    • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android: Native
    • Android: mWeb Chrome
    • iOS: Native
    • iOS: mWeb Safari
    • MacOS: Chrome / Safari
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I followed proper code patterns (see Reviewing the code)
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))
  • If new assets were added or existing ones were modified, I verified that:
    • The assets are optimized and compressed (for SVG files, run npm run compress-svg)
    • The assets load correctly across all supported platforms.
  • If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
    • I verified that all the inputs inside a form are aligned with each other.
    • I added Design label and/or tagged @Expensify/design so the design team can review the changes.
  • I added unit tests for any new feature or bug fix in this PR to help automatically prevent regressions in this user flow.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.

Screenshots/Videos

Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari

Co-authored-by: Cong Pham <suneox@users.noreply.github.com>
@MelvinBot
MelvinBot requested a review from a team September 13, 2026 02:47
@melvin-bot melvin-bot Bot added Melvin-Test-Android Melvin-Test-Web Triggers Melvin to run the testing steps of the PR on web labels Sep 13, 2026
@MelvinBot

Copy link
Copy Markdown
Contributor Author

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:

git remote add upstream https://github.com/Expensify/App.git
git fetch upstream claude-restrictNonOwnerUnreportedExpenseEdit
git checkout -b claude-restrictNonOwnerUnreportedExpenseEdit upstream/claude-restrictNonOwnerUnreportedExpenseEdit
git push -u origin claude-restrictNonOwnerUnreportedExpenseEdit

Then close this PR and open a new one from your fork.

@MelvinBot

MelvinBot commented Sep 13, 2026

Copy link
Copy Markdown
Contributor Author

The test (job 4) failure is not caused by this PR — it is a pre-existing break on main that is currently red on every open PR. I did not push a change.

What's failing: three tests in tests/navigation/NavigateTests.tsx:

  • Navigate › on the narrow layout › returns to the Workspaces list after directly opening a Workspace subpage without its sidebar
  • Navigate › on the narrow layout › removes the sidebar marker when the Workspace split navigator is already mounted
  • Navigate › on the wide layout › keeps the Workspace sidebar while removing the internal sidebar marker

All three assert on routes.at(4) and get undefined. This PR only touches src/libs/ReportUtils.ts and tests/unit/ReportUtilsTest.ts — no navigation code.

Already tracked and already being fixed: #100987 (Workflow Failure, opened against main, same test (job 4)), with two open fix PRs: #101024 and #101016. Both are test-only and take the same approach — the tab fixtures are missing the new Insights route, so the Workspace navigator sits at index 5, not 4.

Nothing to do here. Once either fix PR merges, merge main into this branch and test (job 4) will go green.

Evidence

The same three assertions fail identically on unrelated open PRs:

PR test (job 4)
#101029 (this one) fail
#101027 fail — same 3 test names
#101025 fail
#101022 fail

#100987 attributes the break on main to PR #100387.

The other red check, Check independent approval, is a review-process gate, not a code failure — it needs a human approver, not a commit.


view run · no recording available

@MelvinBot

MelvinBot commented Sep 13, 2026

Copy link
Copy Markdown
Contributor Author

🤖 Android test steps verification results: ⚠️ partial

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.

Category row opens picker for owner, confirming it is not disabled

Step Status
1. As user A, open your self-DM and create a manual track expenseUsed Create expense → Manual, recipient "Tester Tester (you) / Your space", Amount $25.00, Merchant "Test Merchant PR101029". Expense appeared as an Unreported track expense in the self-DM.
Manual track expense created in self-DM
2. In that expense, mention user B and choose "Invite to chat only"Not run. Requires a real second-user account to mention and later sign in as. Using a placeholder email would send a real invite with no way to act as that user afterward.
3. As user B, open user A's track expenseNot run — depends on step 2 and requires operating as a second signed-in identity.
4. Verify Description, Category, Tag, Billable and Report are read-only for user B, and the "Unexpected error creating this chat" message never appearsNot run. This is the core regression check for the fix and categorically requires a second user's perspective.
5. As user A, verify those fields are still editable for youDescription opened an editable "What's it for?" field; text saved and persisted (confirmed by the "set the description to…" system message). Category was tappable and opened its picker. Report and Date rendered as enabled buttons. Tag and Billable are not present on a personal self-DM expense — expected, since they need workspace configuration. Caveat: this covers the owner side in isolation, not preceded by the step-2 invite.
Owner view showing Description edited and saved, Category/Report/Date rows interactive
Category row opens picker for owner, confirming it is not disabled
⚠️
6. Regression: as a domain admin, open an employee's unreported managed-card transaction, tap Report, verify the report-selection step opens instead of /not-foundNot run. Requires a domain-admin identity plus a separate employee account with a managed-card transaction.

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

@MelvinBot

MelvinBot commented Sep 13, 2026

Copy link
Copy Markdown
Contributor Author

🤖 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.

Browser navigated off the app to the public expensify.com marketing page

Step Status
1. As user A, open your self-DM and create a manual track expenseOpened Create Expense → Manual, selected "Melvin Tester (you)" as recipient and entered amount 42.50. Before Merchant/Description could be filled, a "Referral" panel ("Submit an expense, refer your team") appeared over the form; tapping its Back control navigated the whole page to the public expensify.com accountants page, ending the session.
Referral promo overlay appearing over the in-progress Create Expense form
Browser navigated off the app to the public expensify.com marketing page
2. In that expense, mention user B and choose "Invite to chat only"Not run — the expense was never created before the session ended.
3. As user B, open user A's track expenseNot run — no second account is obtainable in this session. This step could not have run even without the navigation failure.
4. Verify Description, Category, Tag, Billable and Report are read-only for user B, and the "Unexpected error creating this chat" message never appearsNot run — depends on steps 2 and 3.
5. As user A, verify those fields are still editable for youNot run — the expense was never created.
6. Regression: as a domain admin, open an employee's unreported managed-card transaction, tap Report, verify report-selection opens instead of /not-foundNot run — the session ended first, and this also needs a domain-admin identity plus a separate employee account with a managed-card transaction.

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

@suneox

suneox commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. You're on a roll.

Reviewed commit: 63f7c7d115

ℹ️ 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".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Melvin-Test-Android Melvin-Test-Web Triggers Melvin to run the testing steps of the PR on web

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants