[Internal QA] Fix From filter showing Account Managers on customer accounts#88921
[Internal QA] Fix From filter showing Account Managers on customer accounts#88921allgandalf wants to merge 48 commits into
Conversation
|
@Krishna2323 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] |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 25f62cef64
ℹ️ 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".
| searchContext: CONST.SEARCH_SELECTOR.SEARCH_CONTEXT_GENERAL, | ||
| initialSelected: initialSelectedOptions, | ||
| excludeLogins: CONST.EXPENSIFY_EMAILS_OBJECT, | ||
| excludeFromSuggestionsOnly: expensifyTeamExclusions, |
There was a problem hiding this comment.
Restrict Expensify-team exclusions to From popup
excludeFromSuggestionsOnly is now always applied in UserSelectPopup, but this popup is shared by ASSIGNEE, ATTENDEE, TO, and FROM in useSearchFiltersBar.tsx (cases around lines 354-357). On wide-screen Reports filters, this change hides @expensify.com/@team.expensify.com people from all those dropdowns instead of only From, which is a regression from the intended behavior. Because includeUserToInvite is false here, users also lose the manual fallback path in these popups.
Useful? React with 👍 / 👎.
…ount-managers # Conflicts: # src/components/Search/FilterDropdowns/UserSelectPopup.tsx # src/components/Search/SearchFiltersParticipantsSelector.tsx
…ount-managers # Conflicts: # android/app/build.gradle # docs/redirects.csv # ios/NewExpensify/Info.plist # ios/NotificationServiceExtension/Info.plist # ios/ShareViewController/Info.plist # package-lock.json # package.json # patches/@shopify/flash-list/details.md # src/components/ReportActionItem/MoneyReportView.tsx # src/components/ReportActionItem/MoneyRequestView.tsx # src/components/Search/FilterComponents/UserSelector.tsx # src/components/Search/index.tsx # src/hooks/useSearchSections.ts # src/libs/Navigation/linkingConfig/RELATIONS/WORKSPACE_TO_RHP.ts # src/libs/Navigation/types.ts # src/libs/actions/IOU/index.ts # src/pages/inbox/report/ContextMenu/BaseReportActionContextMenu.tsx # src/pages/iou/request/step/IOURequestStepConfirmation.tsx # src/pages/workspace/AccessOrNotFoundWrapper.tsx # src/pages/workspace/WorkspaceInitialPage.tsx # src/pages/workspace/WorkspacePageWithSections.tsx # src/pages/workspace/expensifyCard/WorkspaceExpensifyCardDetailsPage.tsx # src/types/onyx/Policy.ts # tests/ui/MoneyReportViewTest.tsx # tests/ui/MoneyRequestViewTest.tsx # tests/unit/PolicyUtilsTest.ts
|
|
neil-marcellini
left a comment
There was a problem hiding this comment.
Thanks for the updates. This is looking much more simple now, and I think it's pretty close. I have reviewed all of it now.
While reviewing the tests, I started wondering if this would break suggestions for filtering on expenses from individuals that were sent outside of a workspace, such as in the P2P path. I believe those users will no longer show up as suggestions, so the approach that was recommended to limit only to workspace members was not a great idea after all.
Also, I see that you've explained how account managers are added to the workspace with their role set to admin. I didn't know that. Filtering them out based on workspace membership does not really work at all, so what we're relying on is basically the existing filtering idea from the policy members page, where we filter out Expensify team users. I do think that's the best approach for now and it's good to be consistent with that page.
One more edge case based on the members page logic. What if the user is a not on the expensify team but they are a member of an expensify team owned policy? Right now for search filter suggestions we would not show any of the expensify policy members even though we should, but we would show all members on the policy page. Let's fix that too.
App/src/pages/workspace/WorkspaceMembersPage.tsx
Lines 432 to 439 in 13b81ae
|
@neil-marcellini thanks, addressed all three:
|
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! ℹ️ 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". |
…ount-managers # Conflicts: # android/app/build.gradle # ios/NewExpensify/Info.plist # ios/NotificationServiceExtension/Info.plist # ios/ShareViewController/Info.plist # package-lock.json # package.json # src/components/Search/FilterComponents/UserSelector.tsx # src/components/Search/index.tsx # src/pages/workspace/WorkspacesListPage.tsx
neil-marcellini
left a comment
There was a problem hiding this comment.
Great work, I'm happy with it!
|
@allgandalf we need to fix merge conflicts. I will also work on getting a contributor plus to review it. |
|
Will review today. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid_hybrid.mp4Android: mWeb Chromeandroid_mWeb.mp4iOS: HybridAppios_hybrid.mp4iOS: mWeb Safariios_safari.mp4MacOS: Chrome / Safariweb_chrome.mp4 |
Krishna2323
left a comment
There was a problem hiding this comment.
LGTM and works well!
@allgandalf please resolve the conflicts when you get a chance. Thanks!
Explanation of Change
the From filter on Reports was leaking current and former AMs because hidden AMs are written into customer
policy.employeeListwithrole: admin, and the filter pulled options from hydrated personal details with no Expensify-team filtering. addedgetExpensifyTeamExclusionsinPolicyUtilsthat soft-excludes@expensify.com/@team.expensify.comlogins from search filter suggestions, mirroring the ruleWorkspaceMembersPagealready uses: skip the filter when the current user is themselves Expensify staff, and preserve Expensify-team members of any Expensify-owned policy the current user belongs to. applied to all user filters (TO, FROM, ASSIGNEE, ATTENDEE) plus inlinefrom:autocomplete.Fixed Issues
$ #83879
PROPOSAL:
Tests
Precondition:
Or run the following CLI command in local VM:
Verify that you don't see any AM or guides in the filter list
Before the fix:
After the Fix:
Offline tests
N/A
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Same as Tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, 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.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.