[Search v1] Add sorting #42248

Kicu · 2024-05-16T07:36:00Z

Details

implements sorting of Search table alongside updating page URL and api calls
new arrow icons are taken from Figma
I cleaned up types for Routes and navigation - they look much nicer now
the added formatted** fields are so that we are guaranteed that the values we sort by are the same as values that are actually displayed for the user, so I compute them in the getSections function; date needs a bit of special handling because we need different date representation for sorting and different for display

Fixed Issues

$ #39892
$ #42269
PROPOSAL:

Tests

Open Search view, either via: Settings > Troubleshoot > New Search Page (NOTE: right now search works only on staging api) or go to [...]/search/all URL
See the search results appearing in a table
click on table headers to toggle between ascending and descending sorting and verify that results are being sorted correctly
check in Network tab whether api calls with proper sort params are sent
there is no sorting on mobile view at the moment
test fix for visual bug: [HOLD for payment 2024-06-11] [Held PR #42248] [Search v1] Date and Merchant value text isn't line with each other in the row #42269 - see if date and merchant value are aligned together

Offline tests

QA Steps

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Android: Native

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

rec-web-sorting-v2.mp4

MacOS: Desktop

src/components/Search.tsx

Kicu · 2024-05-17T12:58:14Z

@luacmartins
I have a question about sorting function:
In the design doc there is an assumption that the sorting function accepts column for sorting and does something like a[columnName] - b[columnName].

This is my first approach to this: https://github.com/Expensify/App/pull/42248/files#diff-4e6d1f540181229a6bc7bd2f96266c9b76bc05cf86150ea1e225c657d358aafbR54

However there are 2 problems with this approach:

the names of columns do not map to property names (solvable via a mapping object which I did)
some of the values that we show in cells are composite values based on more than 1 prop

Examples:

Total is either item.modifiedAmount or item.amount
Description vs Merchant are two fields which are interchangeable
From/To can be either: participant.name, participant.displayName or participant.login

Currently I see no other reasonable solution than a chain of if (columnName === 'from') { // do special user sorting however a negative of this approach is that the sorting is not really agnostic and working on field, but we are leaking the same display logic we use inside TransactionItem into Search component.

Do you see any other solution and what way do you want me to move forward?
Second question: are all fields sortable or only some of them?

Feel free to check the draft PR - everything else should be working

melvin-bot · 2024-05-20T12:11:12Z

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

Kicu · 2024-05-20T14:35:44Z

@shawnborton in here is the fix for #42269

I added this to "test steps" for the reviewer.

github-actions · 2024-05-20T14:40:15Z

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

Android 🤖	iOS 🍎
❌ FAILED ❌	https://ad-hoc-expensify-cash.s3.amazonaws.com/ios/42248/index.html
The QR code can't be generated, because the android build failed
Desktop 💻	Web 🕸️
https://ad-hoc-expensify-cash.s3.amazonaws.com/desktop/42248/NewExpensify.dmg	https://42248.pr-testing.expensify.com

👀 View the workflow run that generated this build 👀

shawnborton · 2024-05-20T14:52:19Z

I feel like when the Search page first loads, we are technically sorting by date right? So as such, I would think we would have this as the starting state:

shawnborton · 2024-05-20T14:54:36Z

For Type and Action, I kinda feel like we don't need a disabled cursor there - let's just use a regular cursor.

Also, some how the header title for the Actions column got messed up - I think that should be centered:

shawnborton · 2024-05-20T14:57:02Z

I don't have any more visual bugs to report (cc @Expensify/design in case you run into anything though), but one thing I did notice was that it doesn't feel like the sorting behavior is actually correct. For instance, if I sort by amount - descending, it seems to be okay but then as I scroll down the list, I see some larger amounts start to appear and it feels like the sorting almost starts over again. Any ideas what's going on there?

dannymcclain · 2024-05-20T15:03:35Z

I feel like when the Search page first loads, we are technically sorting by date right? So as such, I would think we would have this as the starting state:

Agree!

For Type and Action, I kinda feel like we don't need a disabled cursor there - let's just use a regular cursor.

I actually think you should be able to sort by action and type. For example, I might want to sort by action so that all the expenses with a "Pay" button show in a group. Same with type. I don't see why we wouldn't let you sort by those two.

but one thing I did notice was that it doesn't feel like the sorting behavior is actually correct.

Same here. I'm noticing this with most of the sortable fields, not just Total.

shawnborton · 2024-05-20T15:09:47Z

I don't see why we wouldn't let you sort by those two.

Yeah actually that's a totally valid point, I agree with you!

Kicu · 2024-05-28T12:36:00Z

Guys, I will not be merging and solving conflicts on this yet, as this PR: #42410 also introduces big changes to TransactionListItem so I will wait for this one to be merged, and then follow up with sorting.

Meanwhile we can still investigate if sorting logic works correctly

c3024 · 2024-05-28T12:53:34Z

I just checked again. To and Total sorts work well now.

toAndTotalSort.mp4

However, (unrelated to sorting) when I scroll back up the app crashed. There are some undefined from as well apparently.

luacmartins · 2024-05-28T14:15:09Z

that is based on api data and decision made by @luacmartins (https://github.com/Expensify/App/blob/main/src/components/SelectionList/TransactionListItem.tsx#L191)

Yea, this is fine. Not all transactions will have a merchant (that's not the default ones, which we don't want to show).

Distance is shown as merchant

I also think this is fine for now. We can filter this out later if we decide to.

About sorting by to and total this is not something that I understand and I do not have the means to reproduce it.
Carlos is there any way to get an account that would have hundreds of transaction items? I'm looking at the logic for sorting by total and by to and I really see nothing wrong with the code. I have no idea why suddenly the sorting breaks.

The from and to fields have a different sorting logic in the backend (we're sorting by accountID) and we'll work on a solution to sort by from.displayName ?? from.login ?? '' as the frontend does. For now, this is expected.

I'm not sure what the issue with total is. @c3024 what data are you getting for those transactions that are not sorted properly? Also, I'd say we don't block on this and address this in a follow up, since it seems like an edge case.

c3024 · 2024-05-28T14:36:09Z

what data are you getting for those transactions that are not sorted properly?

I cannot repro it today. I didn't see what the transaction data was yesterday.

Also, I'd say we don't block on this and address this in a follow up, since it seems like an edge case.

Agreed. Code changes look good. Will test again after the conflicts are fixed and will complete the checklist.

luacmartins · 2024-05-28T19:20:10Z

@Kicu I merged the ReportListItem PR, can you resolve conflicts now please?

c3024 · 2024-05-29T11:49:15Z

There is a require cycle with PR #42410

WARN Require cycle: src/libs/SearchUtils.ts -> src/components/SelectionList/Search/ReportListItem.tsx -> src/libs/SearchUtils.ts

Nothing broken but we might consider fixing that as well here.

Kicu · 2024-05-29T14:38:27Z

@luacmartins @c3024
This is ready for re-review. Unfortunately a lot has changed when I was resolving the conflicting files and so there is some new code.
Every commit before merges is unchanged - but the last 3 commits are new.

The biggest changes are:

getSortedSections is now a generic function that handles specific types
report items do not have any sorting yet
pieces of code are reused between components

@luacmartins for now the Receipt column is not sortable because I wasn't sure by which field to do the sorting.
If you can tell me we can add it, to do so:

in SearchTableHeader remove the isSortable: false flag
in SearchUtils update the mapping: columnNamesToSortingProperty, introduce new property if needed

Please review :)

luacmartins · 2024-05-29T21:58:26Z

Thanks @Kicu! Not sure if we wanna sort by receipts or not, but we can definitely go ahead without sorting that for now. As for reports, we landed on no sorting for now, so all good there.

luacmartins · 2024-05-29T22:14:56Z

LGTM this is still a thing that we should address in a follow up

luacmartins

LGTM

luacmartins · 2024-05-29T22:24:42Z

@c3024 can you review again please?

c3024 · 2024-05-30T03:23:27Z

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

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

addSort.mp4

MacOS: Desktop

addSortDesktop.mp4

c3024

LGTM!

c3024 · 2024-05-30T03:37:53Z

There are cases of undefined for from as well and app crashed on scrolling up accessing displayName on from. Apart from that, everything looks good.

OSBotify · 2024-05-30T04:21:08Z

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

OSBotify · 2024-05-30T21:54:29Z

🚀 Deployed to staging by https://github.com/luacmartins in version: 1.4.77-11 🚀

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

OSBotify · 2024-05-30T22:30:59Z

🚀 Deployed to staging by https://github.com/luacmartins in version: 1.4.77-11 🚀

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

OSBotify · 2024-06-04T13:57:01Z

🚀 Deployed to production by https://github.com/mountiny in version: 1.4.78-5 🚀

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

WojtekBoman reviewed May 16, 2024

View reviewed changes

src/components/Search.tsx Outdated Show resolved Hide resolved

WojtekBoman approved these changes May 16, 2024

View reviewed changes

Kicu force-pushed the kicu/search-v2/sorting branch from b07ba13 to 0dea1c3 Compare May 16, 2024 12:02

Kicu added 4 commits May 17, 2024 09:13

Refactor SearchTableHeader to allow for sorting by column

c553e4b

Implement search sorting and update page params

48abb83

Add small improvements to Search sorting

b18bfa1

Fix text not aligned in TransactionListItem

b489783

Kicu force-pushed the kicu/search-v2/sorting branch from 8c435c6 to fe5cd89 Compare May 17, 2024 07:42

Update search to work with pagination

62dc109

Kicu force-pushed the kicu/search-v2/sorting branch from fe5cd89 to 62dc109 Compare May 17, 2024 12:06

Fix navigation types

031fe03

Kicu mentioned this pull request May 17, 2024

[HOLD for payment 2024-06-11] [$250] [Search v1] Implement sorting #39892

Open

Kicu marked this pull request as ready for review May 20, 2024 12:11

Kicu requested review from a team as code owners May 20, 2024 12:11

melvin-bot bot removed the request for review from a team May 20, 2024 12:11

melvin-bot bot requested a review from rayane-djouah May 20, 2024 12:11

Make sorting work for every Search column

6e379da

Kicu force-pushed the kicu/search-v2/sorting branch from 8750a91 to 6e379da Compare May 20, 2024 12:19

Kicu mentioned this pull request May 20, 2024

[HOLD for payment 2024-06-11] [Held PR #42248] [Search v1] Date and Merchant value text isn't line with each other in the row #42269

Open

6 tasks

shawnborton added the Ready To Build label May 20, 2024

Kicu added 3 commits May 29, 2024 15:41

Merge branch 'main' into kicu/search-v2/sorting

767373f

Merge branch 'main' into kicu/search-v2/sorting

7660bd9

Fix receipt header missing after merge conflicts

2fd1c57

Kicu force-pushed the kicu/search-v2/sorting branch from 6f5946b to 2fd1c57 Compare May 29, 2024 14:34

Kicu requested review from c3024 and luacmartins May 29, 2024 14:38

luacmartins approved these changes May 29, 2024

View reviewed changes

c3024 approved these changes May 30, 2024

View reviewed changes

melvin-bot bot requested a review from luacmartins May 30, 2024 03:27

luacmartins approved these changes May 30, 2024

View reviewed changes

luacmartins merged commit 9e9d8cd into Expensify:main May 30, 2024
17 checks passed

luacmartins mentioned this pull request May 30, 2024

[$250] [Search v1] Polish items for sorting #42856

Open

2 tasks

github-actions bot mentioned this pull request May 30, 2024

Deploy Checklist: New Expensify 2024-05-30 #42872

Closed

95 tasks

Kicu mentioned this pull request Jun 3, 2024

[Search v1] Add small followups for sorting in search #42980

Merged

49 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Search v1] Add sorting #42248

[Search v1] Add sorting #42248

Kicu commented May 16, 2024 •

edited

Kicu commented May 17, 2024

melvin-bot bot commented May 20, 2024

Kicu commented May 20, 2024

github-actions bot commented May 20, 2024

shawnborton commented May 20, 2024

shawnborton commented May 20, 2024

shawnborton commented May 20, 2024

dannymcclain commented May 20, 2024

shawnborton commented May 20, 2024

Kicu commented May 28, 2024 •

edited

c3024 commented May 28, 2024 •

edited

luacmartins commented May 28, 2024

c3024 commented May 28, 2024 •

edited

luacmartins commented May 28, 2024

c3024 commented May 29, 2024 •

edited

Kicu commented May 29, 2024

luacmartins commented May 29, 2024

luacmartins commented May 29, 2024

luacmartins left a comment

luacmartins commented May 29, 2024

c3024 commented May 30, 2024 •

edited

c3024 left a comment

c3024 commented May 30, 2024 •

edited

OSBotify commented May 30, 2024

OSBotify commented May 30, 2024

OSBotify commented May 30, 2024

OSBotify commented Jun 4, 2024

[Search v1] Add sorting #42248

[Search v1] Add sorting #42248

Conversation

Kicu commented May 16, 2024 • edited

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Kicu commented May 17, 2024

melvin-bot bot commented May 20, 2024

Kicu commented May 20, 2024

github-actions bot commented May 20, 2024

shawnborton commented May 20, 2024

shawnborton commented May 20, 2024

shawnborton commented May 20, 2024

dannymcclain commented May 20, 2024

shawnborton commented May 20, 2024

Kicu commented May 28, 2024 • edited

c3024 commented May 28, 2024 • edited

luacmartins commented May 28, 2024

c3024 commented May 28, 2024 • edited

luacmartins commented May 28, 2024

c3024 commented May 29, 2024 • edited

Kicu commented May 29, 2024

luacmartins commented May 29, 2024

luacmartins commented May 29, 2024

luacmartins left a comment

Choose a reason for hiding this comment

luacmartins commented May 29, 2024

c3024 commented May 30, 2024 • edited

Reviewer Checklist

Screenshots/Videos

c3024 left a comment

Choose a reason for hiding this comment

c3024 commented May 30, 2024 • edited

OSBotify commented May 30, 2024

OSBotify commented May 30, 2024

OSBotify commented May 30, 2024

OSBotify commented Jun 4, 2024

Kicu commented May 16, 2024 •

edited

Kicu commented May 28, 2024 •

edited

c3024 commented May 28, 2024 •

edited

c3024 commented May 28, 2024 •

edited

c3024 commented May 29, 2024 •

edited

c3024 commented May 30, 2024 •

edited

c3024 commented May 30, 2024 •

edited