-
Notifications
You must be signed in to change notification settings - Fork 2.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add display name replacement and make search page router editable #49838
base: main
Are you sure you want to change the base?
add display name replacement and make search page router editable #49838
Conversation
While working on this issue, I've stumbled upon two problems with the current Search implementation of filters: from, to, in, cardID, taxRate:
Changing the API to support more than just IDs may be unavoidable, especially considering the first problem. It would also simplify frontend logic substantially by removing functions that replace IDs with names and vice-versa. |
Agreed. I created an issue for it here and I'll work on it next week. |
@luacmartins The current implementations of advanced filters How should we handle the case of the Moreover, how do we approach the situation where no workspace is selected through the workspace switcher? |
I think this issue will be solved with autocomplete. I was working on the backend PR to allow emails/names but I ran into the same issue you described. I think allowing from/to to use emails is fine because those are unique. I think for now, we'll update from, to and taxRate to take in emails/value and leave cardID and in as is, until autocomplete is available. |
I think implementing @luacmartins can you expand a bit how would autocomplete help the rest of the cases? I understand that we can save the specific Would we not allow user to type in just a string like
I think we should start thinking about these, because not having a single unique identifier (that is not id) for these fields is a bigger problem. One interesting thing we found out yesterday while testing app. You can only type in room mentions in the Composer if you are in a specific workspace (either selected by ws switcher or a workspace report). If you're in "All" room and normal chat - room mentions don't work. rec-room-mention.mp4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some generic comments, I think you could spent a bit of time to simplify the component a bit, since Search is growing quite fast 😅
@rayane-djouah 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] |
@rayane-djouah let's prioritize this PR next since I think we should have this functionality in place before we enable the feature to all users. |
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeiOS: NativeiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BUG: when changing status the header is updated with the search query
- Perform a custom search
- Change the status
- Note that the header changes away from the Search bar and shows the query instead
Expected behavior: keep the search bar with the new status
Screen.Recording.2024-10-04.at.9.53.16.AM.mov
Also, I'm not sure about this one, but we're changing the user input to either add things like Screen.Recording.2024-10-04.at.9.59.39.AM.mov |
When user enters the query we navigate to SearchPage with a different root. Then new input is generated using url making it the only source of truth. This is the reason why user input changes after submit. I can make following changes:
|
@289Adam289 I don't think we need to tackle that in this PR. I realize that it'd involve bigger changes and we need to align on the expected behavior. Let's not block this PR on that or make changes now. |
Ok I will fix the first bug. Should I leave the |
Yea, I think we can do that for now as we think of a more holistic approach to not change the user input |
@luacmartins Currently on main when user changes status via status Bar Search Page resets to canned Query. Is it an expected behavior or do we leave the custom query and replace status with status selected from statusBar? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BUG: App crashes when searching for a non-existent type and then clicking on the "Filters" button
- Perform a custom search with a non-existent type, for example,
type:exp status:all date>2024-10-01
- Click on the "Filters" button.
- Notice that the app crashes.
Screen.Recording.2024-10-07.at.12.33.47.AM.mov
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BUG: App crashes when searching for a non-existent expense type and then selecting an expense type from the filters page
- Perform a custom search with a non-existent expense type, for example,
type:expense status:all expenseType:abcd
- Click on the "Filters" button.
- Click on the expense type menu
- Notice that the app crashes.
Screen.Recording.2024-10-07.at.12.55.26.AM.mov
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Problem: The app does not validate search filters before saving them in Onyx. When certain malformed or incorrect filter values are used (e.g., type:expense status:all date<abcd date>abcd amount>abcd amount<abcd expenseType:abc,abcd currency:abcd from:abc,abcd to:abc,abcd category:abc,abcd tag:abc,abcdd taxRate:abc,abc
), it leads to inconsistent behavior:
- Date Filter: Entering invalid dates results in
NaN
values displayed in the calendar and console warnings.
- Currency Filter: Invalid currencies are shown in the filter page title but not included as selected options on the actual currency filter page
Screen.Recording.2024-10-07.at.1.07.11.AM.mov
- Total filter: Total input fields are populated with the string
NaN
when invalid data is entered.
Screen.Recording.2024-10-07.at.1.09.02.AM.mov
- Category and Tag Filters: Non-existent categories and tags are shown in the filter page title and are included as selected options in their respective filter pages. (I think that this is a correct behaviour)
Screen.Recording.2024-10-07.at.1.10.42.AM.mov
From
andTo
filters: Non-existentFrom
andTo
login values are displayed as empty strings, separated by commas
Screen.Recording.2024-10-07.at.1.12.31.AM.mov
- Tax Rate Filter: Tax rate names that do not exist are ignored
Screen.Recording.2024-10-07.at.1.14.10.AM.mov
In Filter: When entering the input value Screen.Recording.2024-10-07.at.1.35.01.AM.mov |
I think correct behavior would be to ignore incorrect values in advanced filters. It maybe confusing for a user to see an option and to be unable to select it later. Also in case of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BUG: The status bar incorrectly displays a loading skeleton every time the input query is modified and the enter key is pressed, even if the type
has not been changed
- Perform a custom search, for example,
type:expense status:all test
- Modify the search input without changing the
type
, for example, change totype:expense status:all test2
- Observe that the status bar displays a loading skeleton.
Expected behavior: The status bar should only display a loading skeleton if the type
in the query is changed
Screen.Recording.2024-10-07.at.2.18.58.PM.mov
<SearchRouterInput | ||
value={text} | ||
setValue={() => {}} | ||
value={value} | ||
setValue={setValue} | ||
onSubmit={onSubmit} | ||
updateSearch={() => {}} | ||
disabled | ||
isFullWidth | ||
wrapperStyle={[styles.searchRouterInputResults, styles.br2]} | ||
wrapperFocusedStyle={styles.searchRouterInputResultsFocused} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to disable autoFocus
for the TextInput
within the SearchRouterInput
when it is used in the SearchPageHeader
Screen.Recording.2024-10-07.at.2.38.40.PM.mov
I think to allow for parsing special characters like in #49838 (comment) fundamental changes to grammar are inevitable. In previous problems with special character we could just wrap user input in quotation marks. Right now it is impossible without initial parsing. Maybe we should handle it as a follow up. |
Addressing #49838 (comment) in a follow up sounds good to me 👍 |
I added last search type to SearchContext so that search bar skeleton visibility depends on last type and not on ref in Search that resets on every navigation. @rayane-djouah could you take a look? |
Details
follow up from #49462 (comment)
makes search page input editable
allows for using filters
to
andfrom
with emailstaxRate
with names andcardID
with bank nameFixed Issues
$#49121
PROPOSAL:
Tests
from
,to
,taxRate
e.g.from:name.name@email.com
On wide screen:
Use search page input
Verify that it is editable and that search works
Verify that no errors appear in the JS console
Offline tests
QA Steps
from
,to
,taxRate
e.g.from:name.name@email.com
On wide screen:
Use search page input
Verify that it is editable and that search works
Verify that no errors appear in the JS console
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
Screen.Recording.2024-10-04.at.10.33.33.mp4
iOS: mWeb Safari
Screen.Recording.2024-10-04.at.10.37.13.mp4
MacOS: Chrome / Safari
Screen.Recording.2024-10-04.at.10.26.37.mp4
MacOS: Desktop
Screen.Recording.2024-10-04.at.10.40.58.mp4