-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
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: NativeScreen.Recording.2024-10-11.at.11.34.40.AM.movAndroid: mWeb ChromeScreen.Recording.2024-10-11.at.11.31.15.AM.moviOS: NativeSimulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-10-11.at.11.22.15.mp4iOS: mWeb SafariSimulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-10-11.at.11.20.41.mp4MacOS: Chrome / SafariScreen.Recording.2024-10-11.at.11.16.40.AM.movMacOS: DesktopScreen.Recording.2024-10-11.at.11.18.15.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: 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 |
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? |
That should be all |
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:
- Initiate a custom search with invalid filter values using the following query:
type:exp status:al 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 in:abc,abcd
- Click on the "Filters" button.
- Observe that despite that no filters are visibly selected, yet the "Reset Filters" and the "Save search" buttons are availabe.
- Click on the "view results" button
- Notice that the app displays results for a subset of the previous query:
type:exp status:al expenseType:abc,abcd from:abc,abcd to:abc,abcd taxRate:abc in:abc,abcd
, these filter values are not valid and were not visibly selected on the filters page
Expected Behavior: The "Reset Filters" and the "Save search" buttons should only be visible if there are visibly active filters. Additionally, the app should not include invalid filters values when we click on the "view results" button.
Screen.Recording.2024-10-09.at.3.21.11.PM.mov
src/pages/Search/SearchAdvancedFiltersPage/SearchFiltersExpenseTypePage.tsx
Show resolved
Hide resolved
Related to #49838 (comment): The functions For example, if we select a date and a currency from the filters page and view the results, we get Screen.Recording.2024-10-10.at.10.44.31.PM.movI think we can either fix the |
Order changing of standard filters and order changing of keywords are caused by two different things but fixes may interact with each other. I think it would be better to handle both in a follow up pr. |
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.
LGTM and tests well
@luacmartins We're not handling tax names on the backend yet. If I have tax rates in Onyx and search for Screen.Recording.2024-10-11.at.11.27.04.AM.mov |
Ah yea, we only handle the id or tax rate amount, e.g. 5%. I'm not sure if we can support the name, since we can't really differentiate the name from id. I guess we could search a tax by both, id and name whenever an input other than a rate is entered. |
I added the follow up issues to this tracking issue for now #50250. Gonna create individual issues once the dust settles a bit :) |
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.
LGTM. Left a few comments but I think we can address them in a follow up.
.filter((expenseType) => Object.values(CONST.SEARCH.TRANSACTION_TYPE).includes(expenseType as ValueOf<typeof CONST.SEARCH.TRANSACTION_TYPE>)); | ||
} | ||
if (filterKey === CONST.SEARCH.SYNTAX_FILTER_KEYS.CARD_ID) { | ||
filtersForm[filterKey] = filters[filterKey]?.map((card) => card.value.toString()).filter((card) => Object.keys(cardList).includes(card)); |
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.
Same here and for the filters below
filtersForm[filterKey] = filters[filterKey]?.map((card) => card.value.toString()).filter((card) => Object.keys(cardList).includes(card)); | |
filtersForm[filterKey] = filters[filterKey]?.map((card) => card.value.toString()).filter((card) => !!cardList[card]); |
if (filterKey === CONST.SEARCH.SYNTAX_FILTER_KEYS.EXPENSE_TYPE) { | ||
filtersForm[filterKey] = filters[filterKey] | ||
?.map((expenseType) => expenseType.value.toString()) | ||
.filter((expenseType) => Object.values(CONST.SEARCH.TRANSACTION_TYPE).includes(expenseType as ValueOf<typeof CONST.SEARCH.TRANSACTION_TYPE>)); | ||
} |
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.
NAB we can optimize some of these validations
if (filterKey === CONST.SEARCH.SYNTAX_FILTER_KEYS.EXPENSE_TYPE) { | |
filtersForm[filterKey] = filters[filterKey] | |
?.map((expenseType) => expenseType.value.toString()) | |
.filter((expenseType) => Object.values(CONST.SEARCH.TRANSACTION_TYPE).includes(expenseType as ValueOf<typeof CONST.SEARCH.TRANSACTION_TYPE>)); | |
} | |
if (filterKey === CONST.SEARCH.SYNTAX_FILTER_KEYS.EXPENSE_TYPE) { | |
const validExpenseTypes = new Set(Object.values(CONST.SEARCH.TRANSACTION_TYPE)); | |
filtersForm[filterKey] = filters[filterKey]?.filter((value) => value && validExpenseTypes.has(value)); | |
} |
if (filterName === CONST.SEARCH.SYNTAX_FILTER_KEYS.FROM || filterName === CONST.SEARCH.SYNTAX_FILTER_KEYS.TO) { | ||
if (typeof filter === 'string') { | ||
const email = filter; | ||
return PersonalDetailsUtils.getPersonalDetailByEmail(email)?.accountID.toString() ?? filter; | ||
} | ||
const emails = filter; | ||
return emails.map((email) => PersonalDetailsUtils.getPersonalDetailByEmail(email)?.accountID.toString() ?? email); | ||
} |
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.
NAB the API can now handle emails directly, so we could remove this.
Added the follow up above to #50250 |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/luacmartins in version: 9.0.49-0 🚀
|
🚀 Deployed to production by https://github.com/marcaaron in version: 9.0.49-2 🚀
|
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