Only show user from selected workspace filter for From/To filter - #97516
Conversation
|
@thelullabyy 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] |
|
@trjExpensify this PR also makes the Assignee/Attendees filter only show users from the selected workspace filter. Let me know if we don't want that. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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.
|
![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: 7d7830875f
ℹ️ 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".
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-08-06.at.11.10.51.mov |
| logins.add(login); | ||
| } | ||
| } | ||
| return logins; |
There was a problem hiding this comment.
If this set ends up empty we show an infinite loading skeleton instead of "No results found" (areOptionsInitialized in base.ts is length > 0).
It's reachable two ways: employeeList can be blank for a non-active policy, and getAllPolicyValues silently drops policyIDs that aren't in Onyx (saved search / URL for a workspace you no longer have access to). Can we return logins.size ? logins : undefined; so we fall back to all contacts?
What do you think?
There was a problem hiding this comment.
That makes sense. Updated
| const logins = new Set<string>(); | ||
| const selectedPolicies = getAllPolicyValues(policyID, ONYXKEYS.COLLECTION.POLICY, policies); | ||
| for (const policy of selectedPolicies) { | ||
| const employeeLogins = Object.keys(policy.employeeList ?? {}); |
There was a problem hiding this comment.
Raw keys include members with pendingAction: DELETE (just removed, still offline) and members with errors (failed invite). PolicyUtils.getMemberAccountIDsForWorkspace(policy.employeeList, false, false) handles both, and returns accountIDs so we don't have to match on raw login strings.
Do you think we should adding policy.owner too, for parity with isPolicyMember.
There was a problem hiding this comment.
Makes sense, but policy.owner is a login, so we need to convert it to accountID too. If you want, we can wait for #96860, which will add a new mapping of email to personal details. (I want to avoid using getPersonalDetailByEmail since it gets the personal details from Onyx.connect)
There was a problem hiding this comment.
Updated. I decided to keep using the logins. Now employees with pending delete and errors won't show. It's not perfect because deletes can fail, and when that happens, that employee won't be shown. I was planning to only ignore employees with errors if the pending action is ADD, but we remove the pending action when adding the member fails in #79197. The common pattern should keep the pending action when the ADD action fails.
| const allowedContactOptions = includeLoginsOnly ? contactOptions.filter((option) => !!option.login && includeLoginsOnly.has(option.login)) : contactOptions; | ||
| return (defaultOptions ?? []).concat(allowedContactOptions); | ||
| })(); | ||
| const areOptionsInitialized = (optionsWithContacts?.length ?? 0) > 0; |
There was a problem hiding this comment.
With new update, the list can legitimately be empty, "empty" and "not loaded yet" are different states. Could this come from usePersonalDetailOptions's isLoading instead of the length?
There was a problem hiding this comment.
Makes sense. Updated
| // When the workspace filter is set, only suggest the members of the selected workspaces. Users that were already | ||
| // selected when the filter opened are kept in the list even when they aren't members, so they can still be deselected here. | ||
| const workspaceMemberLogins = (() => { | ||
| if (!policyID?.value?.length) { |
There was a problem hiding this comment.
Should we handle the case where policyID.isNegated as well?
There was a problem hiding this comment.
value already contains the selected value, whether negated or not. getAllPolicyValues handles both cases.
Got it. I think that makes sense for spend page filters. 👍 CC: @dannymcclain @JmillsExpensify |
|
Hmm, not sure. I think if we want to do that then we should do it more all workspace features – categories, tags, etc. which isn't currently the case. So I'd default to not doing that right now in this PR, unless we are doing for every single workspace feature. |
We already do the filter by workspace for tag, category, exported to, and tax rate. |
|
Ah ok, then I'm cool with doing it here. |
|
@bernhardoj Let me know if it is ready for another round of review |
|
@thelullabyy please check again |
|
@thelullabyy bump |
| const expensifyTeamExclusions = getExpensifyTeamExclusions(personalDetails, policies, currentUserPersonalDetails.email); | ||
|
|
||
| // Snapshot the pre-selected accountIDs from when the filter first opened so they can be floated to the | ||
| // top on first render without repinning rows that are toggled afterwards (see https://github.com/Expensify/App/issues/61414). |
There was a problem hiding this comment.
NAB: I think we should not link the issue in comment like this, it is really easy to get outdated
There was a problem hiding this comment.
This is an existing comment, actually, but I removed it.
thelullabyy
left a comment
thelullabyy
left a comment
There was a problem hiding this comment.
LGTM but let's update the code-comment here @bernhardoj
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #97343 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 mountiny 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! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.4.52-0 🚀
|
Help site review — changes required ✅This PR limits the Spend page From and To filters to the selected workspace's members when a Workspace filter is applied (users already selected stay in the list so they can be deselected). That user-facing behavior wasn't documented on the help site, so a docs update is warranted. Draft help site PR: #98217 It adds a short @bernhardoj, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR |
8 participants
Explanation of Change
Fixed Issues
$ #97343
PROPOSAL:
Tests
Same as QA Steps
Offline tests
Same as QA Steps
QA Steps
Prerequisite: have some workspaces with some members
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
web.23.26.22.mp4