Skip to content

Do not seed My expenses saved search for approve-only accounts - #97417

Draft
aswin-s wants to merge 2 commits into
Expensify:mainfrom
aswin-s:fix/issue-97213-follow-up
Draft

Do not seed My expenses saved search for approve-only accounts#97417
aswin-s wants to merge 2 commits into
Expensify:mainfrom
aswin-s:fix/issue-97213-follow-up

Conversation

@aswin-s

@aswin-s aswin-s commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Explanation of Change

Follow-up to #93541, which seeded the "My expenses" saved search on approve-only accounts.

The submitter half of isSubmitterAndApprover was derived from isGroupPolicy(policy) — a check on policy.type that never looks at the current user:

isSubmitter = isSubmitter || isGroupPolicy(policy);

The POLICY Onyx collection only holds policies the user belongs to, so this was true for anyone in any group workspace. Eligibility therefore collapsed to "is an approver somewhere", and the approve-only account in the linked issue satisfied it: member of the workspace ⇒ isSubmitter, set as the submitter's approver in workflow ⇒ isApprover.

This also explains why the submit-only case in #93541's test steps passed — only the approve-only half of the condition was broken, so manual testing did not surface it.

This PR gates the first half on the admin/auditor role instead, matching the isAdmin || isAuditor check the admin-facing suggested searches already use in getSuggestedSearchesVisibility. Any member of a group workspace can file their own expenses, so managing a workspace covers the "submits" half of the audience #92780 describes.

Notes for reviewers on why role is the signal used, since two more obvious candidates do not work:

  1. Approval-chain position cannot distinguish these accounts. Being an approver is not recorded on the approver's own employeeList entry — it is an inbound reference from whoever submits to them. The default workflow also gives every member a submitsTo target (members route to the owner, the owner to themselves), so requiring one does not exclude the reported account.
  2. Neither can membership or expense-creation capability. An approve-only account is a full member with its own policy expense chat, so isPolicyExpenseChatEnabled + membership is also true for it. Gating on whether the user has expenses was rejected separately: a newly-added manager with no expenses yet should still be seeded.

Three existing unit tests asserted the old behaviour (including one named "any member is a submitter, not just role=user") and have been corrected.

Fixed Issues

$ #97213
PROPOSAL:

Tests

Preconditions: 3 accounts (1 workspace owner, 1 submitter, 1 approver).

  1. As the owner, create a workspace with Workflows enabled and invite the submitter and the approver as Members.
  2. Go to Workspace settings > Workflows, enable Approvals, and add an approval workflow: expenses from the submitter are approved by the approver.
  3. Log in as the approver and navigate to Search.
  4. Verify no "My expenses" entry appears in the Saved section of the left-hand nav, and no Saved section is shown at all if they have no other saved searches.
  5. Log in as the submitter and navigate to Search. Verify no "My expenses" entry appears.
  6. Log in as the owner (workspace admin, and the default approver) and navigate to Search. Verify a "My expenses" entry does appear in the Saved section, and that opening it filters to from:<owner account ID>.
  7. As the owner, delete the "My expenses" saved search. Sign out, clear site storage, sign back in, and navigate to Search. Verify it does not reappear.

Failure scenario: with the network throttled or offline at step 6, the entry appears optimistically and rolls back if the SaveSearch write fails, matching the existing manual save-search behaviour.

  • Verify that no errors appear in the JS console

Offline tests

Unchanged from #93541. The seed uses the existing saveSearch write path, which has optimistic data plus failure rollback, so offline behaviour is identical to saving a search manually while offline: the entry appears optimistically and syncs when back online. This PR only narrows which accounts are eligible and adds no new network calls.

QA Steps

Same as tests. The key assertion is step 4 — the approve-only account must not receive the seeded search — and step 6, which confirms the eligible dual-role account still does.

  • 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

Verified on a real 3-account workspace against the dev backend (owner + submitter + approver, with a workflow routing the submitter to the approver):

Account Workspace role Recognized as approver "My expenses" seeded
Approver (the reported account) user yes no
Owner / admin admin yes yestype:expense from:<accountID>

For the approver, nvp_hasSeededMyExpensesSearch and nvp_savedSearches both remained unset and no Saved section rendered. For the owner, the NVP was set and the entry appeared in the Saved section.

The submitter half of isSubmitterAndApprover was derived from isGroupPolicy,
a check on policy.type that never looks at the current user. The POLICY Onyx
collection only holds policies the user belongs to, so it was true for anyone
in a group workspace and eligibility collapsed to "is an approver somewhere".

Gate on the admin/auditor role instead, matching the admin-facing suggested
searches. Being an approver is not recorded on the approver's own employee
entry, and the default workflow gives every member a submitsTo target, so role
is the only signal that separates a manager from a plain member who happens to
approve someone.
@aswin-s
aswin-s force-pushed the fix/issue-97213-follow-up branch from f420560 to 19b117e Compare July 30, 2026 01:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant