Fix: include member workspace chats in search router for admins#92226
Fix: include member workspace chats in search router for admins#92226MelvinBot wants to merge 1 commit into
Conversation
Exempt POLICY_EXPENSE_CHAT from the excludeHidden filter in getValidOptions so a workspace chat is not dropped from search results when the current user's notification preference is hidden, mirroring the existing POLICY_ADMINS and POLICY_ROOM exemptions. Co-authored-by: Jayesh Mangwani <jayeshmangwani@users.noreply.github.com>
|
I analyzed the checks on this PR. There's no code fix to push — all CI checks pass. Status: 29 passing, 7 skipped, and 1 failing: The checklist check is failing only because the PR Author Checklist boxes are unticked ( Full check breakdown
Once you complete the checklist, that last check will pass. |
|
@MelvinBot , could you please check all the checkboxes in the PR checklist? I'll add the screenshots and video, and I'll handle the testing. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid.movAndroid: mWeb Chromemweb-chrome.moviOS: HybridAppios.moviOS: mWeb Safarimweb-saafri.movMacOS: Chrome / Safariweb.mov |
|
🤖 Done — I checked all 49 boxes in the PR Author Checklist. I left the two |
|
@MelvinBot Please mark PR as "Ready for review" |
|
🤖 Done — marked this PR as Ready for review. |
|
@jayeshmangwani 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] |
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #91779 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
Explanation of Change
When an admin invites a member to a workspace, the member's workspace chat (
POLICY_EXPENSE_CHAT) is created with the admin'snotificationPreferenceset toHIDDEN(only the invited member is set toALWAYS). This is intentional so quiet workspace chats don't clutter the admin's LHN until there's activity.The search router (chat finder) calls
getSearchOptionswithexcludeHidden: true. InsidegetValidOptions, theexcludeHiddenfilter drops any non-thread/non-task report whose current-usernotificationPreferenceis hidden, unless itschatTypeis in an exemption list. The list exemptedSELF_DM,POLICY_ADMINS,POLICY_ROOM, andGROUP, but notPOLICY_EXPENSE_CHAT— so the admin's hidden preference caused the member's workspace chat to be filtered out of search results.This change adds
POLICY_EXPENSE_CHATto that exemption list, mirroring the existingPOLICY_ADMINS/POLICY_ROOMexemptions, so workspace chats remain discoverable in the search router regardless of the current user's notification preference. The LHN visibility logic inSidebarUtilsis untouched (it has its ownisOwnPolicyExpenseChatoverride), so this does not change LHN behavior. Access is unaffected because non-members never have other users' workspace chats in their Onyx in the first place.Added a unit test in
getSearchOptions()that verifies aPOLICY_EXPENSE_CHATwith a hidden current-user notification preference is still included in the search results.Fixed Issues
$ #91779
PROPOSAL: #91779 (comment)
Tests
// TODO: The human co-author must fill out the tests you ran before marking this PR as "ready for review"
// Please describe what tests you performed that validates your changed worked.
Offline tests
Same as tests.
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
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.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari