Repository navigation
Scope Top Spenders suggested search to eligible workspace policyIDs - #95593
Conversation
Co-authored-by: abdulrahuman5196 <abdulrahuman5196@users.noreply.github.com>
Co-authored-by: abdulrahuman5196 <abdulrahuman5196@users.noreply.github.com>
Co-authored-by: abdulrahuman5196 <abdulrahuman5196@users.noreply.github.com>
Co-authored-by: abdulrahuman5196 <abdulrahuman5196@users.noreply.github.com>
Co-authored-by: abdulrahuman5196 <abdulrahuman5196@users.noreply.github.com>
|
🤖 I created this PR from a branch on the upstream repo ( If you'd like to take ownership of this branch and push updates from your own fork, run: git remote add upstream https://github.com/Expensify/App.git
git fetch upstream claude-topSpendersPolicyScope
git checkout -b claude-topSpendersPolicyScope upstream/claude-topSpendersPolicyScope
git push -u origin claude-topSpendersPolicyScopeThen you can close this PR and open a new one from your fork. Otherwise, feel free to push to this upstream branch directly if you have access. |
|
MelvinBot has made 5 consecutive commits trying to fix CI failures. Automatic CI fix dispatches are now paused for this PR. Next steps: A human should review the recent commits and resolve the conflicting CI errors manually. |
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.
|
|
@MelvinBot Can you merge from main and publish this PR? |
…icyScope Co-authored-by: abdulrahuman5196 <abdulrahuman5196@users.noreply.github.com>
…_POLICY/PERSONAL_POLICY) Co-authored-by: abdulrahuman5196 <abdulrahuman5196@users.noreply.github.com>
|
@abdulrahuman5196 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] |
|
🤖 Done — merged What I did:
A couple of notes:
|
![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: b7a330a13a
ℹ️ 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".
|
@MelvinBot For Tests, Create a manual expense with date from last month and submit it to any individual chat. Mark QA tests as same as Tests and Offline tests as not applicable. Also check all the author's checklist. |
|
🤖 Updated the PR description:
On the author checklist: I checked the items that are truthfully satisfied by this PR — issue linked, testing/QA steps written, code-pattern/guideline items, and unit tests added. I intentionally left the following unchecked because they assert manual work that hasn't been done and that I can't verify (there are also no screenshots in the PR yet), so please complete and tick these yourself:
Ticking those on your behalf would be claiming verification that didn't happen, which would defeat the checklist's purpose — I'd rather flag them than assert them falsely. Once you've done the manual platform testing and added the screenshots, check the remaining boxes so the |
|
@MelvinBot Can you analyse and address this concern - #95593 (comment) ? Seems valid, make the required changes if valid? |
…ves its search key
Co-authored-by: {"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"} <{"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"}@users.noreply.github.com>
|
@heyjennahay this is ready for your look |
|
@MelvinBot Can you fix the merge conflicts and make the exact change alone as suggested here #95593 (comment) and here #95593 (comment) |
Co-authored-by: abdulrahuman5196 <abdulrahuman5196@users.noreply.github.com>
…icyScope Co-authored-by: abdulrahuman5196 <abdulrahuman5196@users.noreply.github.com> # Conflicts: # src/libs/SearchUIUtils.ts
|
🤖 Done — pushed to Merge conflicts: Resolved by merging Review suggestions applied (exactly as suggested, nothing else):
Verification: |
|
🚧 MonilBhavsar 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. |
|
🚀 Deployed to staging by https://github.com/MonilBhavsar in version: 9.4.41-0 🚀
|
Help site review — no changes requiredI reviewed this PR against the help site files under Why: This is a backend query-scoping bug fix. It stops personal / individual-chat expenses from leaking into the Top spenders suggested search by scoping the query to the eligible workspace The one relevant article —
Nowhere does the article claim personal / individual-chat expenses are included, so the fix brings actual behavior in line with what's already documented. No draft PR was created. @abdulrahuman5196, since no help site changes are needed there is no linked docs PR to review. If you believe the article should be updated (e.g. to explicitly note the workspace-scoping), let me know and I'll create a draft PR. |
|
QA team failed this PR on iOS with an original issue 1784608140919.4d4d43f2-160b-4ce6-b63f-5c7b2469fb56.mp4 |
|
The original issue is under review, so I'm going to let this keep going forward. |
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 9.4.41-4 🚀
Bundle Size Analysis (Sentry): |
…pendersPolicyScope" This reverts commit 241cc85, reversing changes made to be5e677. Top Spenders is one of several workspace insights that can leak individual-chat expenses, and the client-side workspace filter made the UX unpredictable. The leakage will be fixed on the backend instead, by excluding expenses on IOU reports from the suggested-search insights.
|
🚀 Deployed to staging by https://github.com/MonilBhavsar in version: 9.4.75-0 🚀
|
|
🤖 Yes — help site changes are required. Draft PR: #100741 This PR scoped Top spenders to eligible workspaces, so expenses submitted in an individual chat no longer appear. The help site never stated which expenses the report covers, so a reader could reasonably still expect personal expenses to be counted. Two articles now document the workspace scope. What changed in the docs PR
Why only Top spenders, and how labels were verifiedOnly the Top Spenders query gained a Scope wording matches the eligibility rule at UI labels were verified against Applied @abdulrahuman5196, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR |
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.4.75-3 🚀
Bundle Size Analysis (Sentry): |
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.4.75-3 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
The "Top Spenders" suggested search (under Spend → insights) built its query with
type:expensegrouped by submitter for the last month, but with nopolicyIDfilter. The backend therefore returned every expense the user is party to — including personal / individual-chat expenses that don't belong to any workspace — which is how an IOU submitted to an individual chat leaked into "Top Spenders".This PR scopes the Top Spenders query to the workspaces that are actually eligible for the section, so only expenses belonging to those workspaces are returned.
What changed in
src/libs/SearchUIUtils.ts:getSuggestedSearchesVisibilitynow also collects the IDs of every policy that passes the Top Spenders eligibility check (isEligibleForTopSpendersSuggestion) and returns them astopSpendersPolicyIDs. The per-policy loop was converted fromArray.some(...)(which short-circuited once all suggestion flags weretrue) to a fullfor…ofiteration so that later eligible policies are always visited and the ID list is complete.getSuggestedSearchesaccepts a newtopSpendersPolicyIDs: string[]argument and, when it is non-empty, addspolicyID: topSpendersPolicyIDsto the Top Spenders query viabuildQueryStringFromFilterFormValues(which serializes it topolicyID:id1,id2). When the list is empty the filter is omitted, so we never emit a query that silently matches nothing.createTypeMenuSectionsthreads the collectedtopSpendersPolicyIDsfrom the visibility helper intogetSuggestedSearches.Note: adding
policyIDto the query changes the resultinghash/recentSearchHash, which is expected — the suggested-search identity now includes the scoped workspaces.Added unit tests covering: eligible policy IDs are collected and scoped into the query, ineligible (personal) workspaces are excluded, and no
policyIDfilter is emitted when there are no eligible workspaces.Fixed Issues
$ #90294
PROPOSAL: #90294 (comment)
Tests
Offline tests
N/A
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, 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