[Desktop Navigation] Add a dedicated Workspace filter to the Reports tab by WojtekBoman · Pull Request #59450 · Expensify/App

WojtekBoman · 2025-04-01T14:57:08Z

Explanation of Change

This PR adds a new Workspace filter to the Reports tab. To display the new screen it's necessary to have access to the leftHandBar beta.

Fixed Issues

$ #59366
PROPOSAL:

Tests

Verify that no errors appear in the JS console

Precondition: You are logged in to an account with access to the leftHandBar beta

Open the Reports tab
Open the filters page
Select Workspace from the filters list
Select any workspace
Select View results
Verify if the results are filtered correctly

Offline tests

QA Steps

Same as tests

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Android: Native

Screen.Recording.2025-04-08.at.13.10.46.mov

Android: mWeb Chrome

Screen.Recording.2025-04-08.at.13.12.44.mov

iOS: Native

Screen.Recording.2025-04-08.at.13.28.47.mov

iOS: mWeb Safari

MacOS: Chrome / Safari

Screen.Recording.2025-04-08.at.13.03.52.mov

MacOS: Desktop

Screen.Recording.2025-04-08.at.13.08.05.mov

Kicu

LGTM 👍 , left you a few comments.

I did not run the code, just went through this to see if its readable and what I would expect

Kicu · 2025-04-04T12:37:48Z

this is temporary right? eventually we need to fallback to something so that we display it for user. Something like "unknown" or simply policyID?

Kicu · 2025-04-04T12:38:40Z

related to above ☝️ if by any chance

policies[`${ONYXKEYS.COLLECTION.POLICY}${workspaceFilter}`]

is undefined, then we have no name and we display nothing

Kicu · 2025-04-04T12:41:02Z

withPermissionCheck sounds super generic and its not easy to know what permission we check.

Change the name to something related to what we are actually implementing. Example: typeFiltersKeysWith[Optional|Possible]Policy etc

Kicu · 2025-04-04T12:44:53Z

can we instead do something like:

const [firstArray, ...rest] = arrays; return [key, [firstArray, policy, rest]

? also it reads quit bad with the generic name firstArray. Maybe we can call it for what it is - like filtersConfig, filtersConfig element(s)?

I renamed it to firstFiltersSection to indicate what it is related to

WojtekBoman · 2025-04-07T13:53:19Z

@Expensify/design @flaviadefaria @mountiny

Hey, it should be ready for testing, feel free to generate new builds and check if the new workspace filter works fine :) Tomorrow I'll put final touches on this PR and release it for review

github-actions · 2025-04-07T13:55:16Z

🚧 @dannymcclain has triggered a test app build. You can view the workflow run here.

github-actions · 2025-04-07T14:26:15Z

🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪

Android 🤖	iOS 🍎
Android 🤖🔄	iOS 🍎🔄
https://ad-hoc-expensify-cash.s3.amazonaws.com/android/59450-hybrid/index.html	https://ad-hoc-expensify-cash.s3.amazonaws.com/ios/59450-hybrid/index.html

Desktop 💻	Web 🕸️
https://ad-hoc-expensify-cash.s3.amazonaws.com/desktop/59450/NewExpensify.dmg	https://59450.pr-testing.expensify.com

👀 View the workflow run that generated this build 👀

dannymcclain · 2025-04-07T14:56:46Z

Looking and working as expected to me!

When I select a workspace in the filter, it changes the header—I assume that's expected for now, right?

Unrelated to this PR, I will note that personally I'd love to see a workspace column for the expenses. Because we're just showing From and To (which are individuals), I kinda just have to trust that the expenses are in fact on the workspace I'm filtering to... There's no real visual confirmation that that is true. But we can talk about that some other time in some other place 😅

shawnborton · 2025-04-07T16:03:30Z

When I select a workspace in the filter, it changes the header—I assume that's expected for now, right?

I would think that if this is part of the updated LHB project, we should not be changing the header when a workspace is selected, right?

shawnborton · 2025-04-07T16:04:26Z

Actually maybe that is expected for now, since the other PR is the one that updates the content/look of the header. So maybe ignore me!

WojtekBoman · 2025-04-08T06:15:54Z

When I select a workspace in the filter, it changes the header—I assume that's expected for now, right?

I would think that if this is part of the updated LHB project, we should not be changing the header when a workspace is selected, right?

Yes, it's expected for now, the workspace icon will be removed in another PR :)

289Adam289

LGTM. Left just one small comment. It may be possible that sanitize checks will be needed, but I wasn't able to find anything else.

289Adam289 · 2025-04-08T09:00:44Z

Workspace name can include space so running it through sanitizeSearchValue is necessary

melvin-bot · 2025-04-08T11:34:19Z

@dukenv0307 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]

dannymcclain · 2025-04-08T13:09:40Z

I think from a design perspective this is good to go 🚀

dukenv0307 · 2025-04-08T16:21:22Z

We only use policies to get the name of specific policyID, so I think we should pass the policyName: string

Hmm, I implemented it similarly to other filters, we pass the entire collection and extract data for specific ids/ids. Also, it will be useful when we implement a multiselect workspace filter

dukenv0307 · 2025-04-08T16:53:39Z

I think it should be !queryJSON || ...

Hmm, I am not sure if isCannedQuery should be true when !queryJSON, maybe I'll reuse code that I wrote in SearchTypeMenu.tsx

dukenv0307 · 2025-04-08T16:57:15Z

Looks good, just left some minor comments

mountiny · 2025-04-08T17:03:35Z

Awaiting for the feedback to be addressed

WojtekBoman · 2025-04-08T17:27:58Z

@dukenv0307 I replied to your comments, please check if it's ok now :)

dukenv0307 · 2025-04-08T17:33:20Z

code looks good now

dukenv0307 · 2025-04-08T17:46:38Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible 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 checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android: Native
- Android: mWeb Chrome
- iOS: Native
- iOS: mWeb Safari
- MacOS: Chrome / Safari
- MacOS: Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- 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 shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- 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)
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
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 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.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
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.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Android: Native

Screen.Recording.2025-04-09.at.00.42.06.mov

Android: mWeb Chrome

Screen.Recording.2025-04-09.at.00.39.38.mov

iOS: Native

Screen.Recording.2025-04-09.at.00.41.01.mov

iOS: mWeb Safari

Screen.Recording.2025-04-09.at.00.38.36.mov

MacOS: Chrome / Safari

Screen.Recording.2025-04-09.at.00.38.02.mov

MacOS: Desktop

Screen.Recording.2025-04-09.at.00.45.09.mov

dukenv0307 · 2025-04-08T17:47:01Z

LGTM

mountiny

@WojtekBoman @dukenv0307 are there no unit tests to add for this new search option?

mountiny · 2025-04-08T18:04:57Z

+/**
+ * A copy of `isDefaultExpensesQuery` handling the policy ID, used if you have access to the leftHandBar beta.
+ * When this beta is no longer needed, this method will be renamed to `isDefaultExpensesQuery` and will replace the old method.
+ *


Suggested change

*

It will be done in this PR: #59943

OSBotify · 2025-04-08T18:28:32Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

github-actions · 2025-04-09T04:40:27Z

🚀 Deployed to staging by https://github.com/mountiny in version: 9.1.25-0 🚀

platform	result
🖥 desktop 🖥	success ✅
🕸 web 🕸	success ✅
🤖 android 🤖	success ✅
🍎 iOS 🍎	success ✅

github-actions · 2025-04-10T00:56:16Z

🚀 Deployed to production by https://github.com/jasperhuangg in version: 9.1.25-4 🚀

platform	result
🖥 desktop 🖥	success ✅
🕸 web 🕸	success ✅
🤖 android 🤖	failure ❌
🍎 iOS 🍎	success ✅

WojtekBoman · 2025-04-10T07:58:16Z

@WojtekBoman @dukenv0307 are there no unit tests to add for this new search option?

I believe the logic of the new filter still needs to be adjusted, it will be done in the next [Desktop Navigation] PRs, so we'll add tests once the whole functionality is finished

mountiny · 2025-04-10T11:38:20Z

thanks

Kicu reviewed Apr 4, 2025

View reviewed changes

WojtekBoman force-pushed the improve-desktop-nav/1-add-workspace-filter branch from 3c4d950 to 24ab5b1 Compare April 7, 2025 13:40

mountiny assigned WojtekBoman Apr 7, 2025

289Adam289 approved these changes Apr 8, 2025

View reviewed changes

WojtekBoman marked this pull request as ready for review April 8, 2025 11:34

WojtekBoman requested a review from a team as a code owner April 8, 2025 11:34

melvin-bot Bot requested review from dukenv0307 and removed request for a team April 8, 2025 11:34

WojtekBoman added 9 commits April 8, 2025 15:20

Add SearchFiltersWorkspacePage

5802fcf

Add Nullable to types utils

08cdc63

Handle unselecting workspace filter

d35eebf

Hide Workspace filter behind leftHandBar beta

767f164

Add SearchQueryUtils methods that handle displaying workspace filter

5d7450c

Cleanup workspace filter code

a27dc82

Fix variable names in typeFiltersKeysWithOptionalPolicy

dc4e6b8

Sanitize workspace filter value

6bc94aa

Adjust comments of copied methods

d5ab426

WojtekBoman mentioned this pull request Apr 8, 2025

[Desktop Navigation] Drop the <Workspace> from the LHN headers #59575

Merged

50 tasks

dukenv0307 reviewed Apr 8, 2025

View reviewed changes

Rewrite isCannedQuery logic in SearchPageNarrow.tsx

09f9c68

WojtekBoman force-pushed the improve-desktop-nav/1-add-workspace-filter branch from a125f4b to 09f9c68 Compare April 8, 2025 17:27

dukenv0307 approved these changes Apr 8, 2025

View reviewed changes

melvin-bot Bot requested a review from mountiny April 8, 2025 17:46

mountiny approved these changes Apr 8, 2025

View reviewed changes

mountiny merged commit 8648481 into Expensify:main Apr 8, 2025
18 checks passed

github-actions Bot mentioned this pull request Apr 9, 2025

Deploy Checklist: New Expensify 2025-04-09 #59866

Closed

66 tasks

WojtekBoman mentioned this pull request Apr 9, 2025

[Due for payment 2025-04-25] [Desktop Navigation] Reports filters - Workspace filter is displayed when account has no workspace #59882

Closed

8 tasks

jponikarchuk mentioned this pull request Apr 9, 2025

[Desktop Navigation] [$250] Reports - Deleted workspace is displayed on workspace filter option and switcher #59914

Closed

8 tasks

melvin-bot Bot mentioned this pull request Apr 10, 2025

[Due for payment 2025-05-14] [Due for payment 2025-04-17] [Desktop Navigation] Add a dedicated Workspace filter to the Reports tab #59366

Closed

Conversation

WojtekBoman commented Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Explanation of Change

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Uh oh!

Kicu left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WojtekBoman commented Apr 7, 2025

Uh oh!

github-actions Bot commented Apr 7, 2025

Uh oh!

github-actions Bot commented Apr 7, 2025

Uh oh!

dannymcclain commented Apr 7, 2025

Uh oh!

shawnborton commented Apr 7, 2025

Uh oh!

shawnborton commented Apr 7, 2025

Uh oh!

WojtekBoman commented Apr 8, 2025

Uh oh!

289Adam289 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

melvin-bot Bot commented Apr 8, 2025

Uh oh!

dannymcclain commented Apr 8, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dukenv0307 commented Apr 8, 2025

Uh oh!

mountiny commented Apr 8, 2025

Uh oh!

WojtekBoman commented Apr 8, 2025

Uh oh!

dukenv0307 commented Apr 8, 2025

Uh oh!

dukenv0307 commented Apr 8, 2025

Reviewer Checklist

Screenshots/Videos

Uh oh!

dukenv0307 commented Apr 8, 2025

Uh oh!

mountiny left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

OSBotify commented Apr 8, 2025

Uh oh!

WojtekBoman commented Apr 1, 2025 •

edited

Loading

Kicu left a comment •

edited

Loading