App icon indicating copy to clipboard operation
App copied to clipboard

[$250] Search - Search loads infinitely when search query contains type:chat and category query

Open IuliiaHerets opened this issue 1 year ago • 39 comments

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Version Number: 9.0.65-1 Reproducible in staging?: Y Reproducible in production?: Y Email or phone of affected tester (no customers): [email protected] Issue reported by: Applause Internal Team

Action Performed:

  1. Go to staging.new.expensify.com
  2. Click search router icon.
  3. Enter the following query: type:chat in: category:car
  4. Hit Enter.

Expected Result:

Search will not load infinitely.

Actual Result:

Search loads infinitely.

Workaround:

Unknown

Platforms:

  • [x] Android: Standalone
  • [x] Android: HybridApp
  • [x] Android: mWeb Chrome
  • [x] iOS: Standalone
  • [x] iOS: HybridApp
  • [x] iOS: mWeb Safari
  • [x] MacOS: Chrome / Safari
  • [x] MacOS: Desktop

Screenshots/Videos

https://github.com/user-attachments/assets/14013d8a-7a42-482d-9a1f-6cee8ab021e1

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021859790661845309370
  • Upwork Job ID: 1859790661845309370
  • Last Price Increase: 2024-12-06
  • Automatic offers:
    • daledah | Contributor | 105274097
Issue OwnerCurrent Issue Owner: @daledah

IuliiaHerets avatar Nov 21 '24 13:11 IuliiaHerets

Triggered auto assignment to @Christinadobrzyn (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

melvin-bot[bot] avatar Nov 21 '24 13:11 melvin-bot[bot]

Edited by proposal-police: This proposal was edited at 2024-11-21T21:22:56Z.

Proposal

Please re-state the problem that we are trying to solve in this issue.

Search loads infinitely.

What is the root cause of that problem?

The API Search with the above query returns error

Screenshot 2024-11-21 at 21 21 53

When Search API fails, we just update the loading to false

https://github.com/Expensify/App/blob/4c44827398f884af626872c33f64dd358d19eff4/src/libs/actions/Search.ts#L45

that make the condition shouldShowLoadingState is still false

https://github.com/Expensify/App/blob/4c44827398f884af626872c33f64dd358d19eff4/src/components/Search/index.tsx#L190-L195

so we can see the loading state

What changes do you think we should make in order to solve the problem?

Solution 1: Add failure data within error in

https://github.com/Expensify/App/blob/4c44827398f884af626872c33f64dd358d19eff4/src/libs/actions/Search.ts#L51

then in Search.tsx, detect if error is in searchResults, we will show the error message

Solution 2: Add failure data within data: [], status, and type

    const failureData = [{
            onyxMethod: Onyx.METHOD.MERGE,
            key: `${ONYXKEYS.COLLECTION.SNAPSHOT}${hash}`,
            value: {
                data: [],
                search: {
                    status,
                    type
                }
            },
    }]

then the empty state will be shown

What alternative solutions did you explore? (Optional)

NA

daledah avatar Nov 21 '24 14:11 daledah

I think this might have to do with how type:chat in:<any chat> category:car is being read? Here's what is showing under keyword, I think this is supposed to be 'chat'?

Snagit 2021 2024-11-22 10 46 13

Christinadobrzyn avatar Nov 22 '24 02:11 Christinadobrzyn

Job added to Upwork: https://www.upwork.com/jobs/~021859790661845309370

melvin-bot[bot] avatar Nov 22 '24 02:11 melvin-bot[bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @alitoshmatov (External)

melvin-bot[bot] avatar Nov 22 '24 02:11 melvin-bot[bot]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Search - Search loads infinitely when search query contains type:chat and category query

What is the root cause of that problem?

The Search api returns jsonCode: 402 and empty onyxData. then https://github.com/Expensify/App/blob/5f483dc37ccfbb967d8eeb49302d3dfa010f93e9/src/components/Search/index.tsx#L190-L195 shouldShowLoadingState is true because searchResults?.search?.status is undefined. According to this below logic, we are showing the infinite loading component. https://github.com/Expensify/App/blob/5f483dc37ccfbb967d8eeb49302d3dfa010f93e9/src/components/Search/index.tsx#L274-L281

What changes do you think we should make in order to solve the problem?

We have to add the failureData in getOnyxLoadingData function.

    const failureData: OnyxUpdate[] = [
        {
            onyxMethod: Onyx.METHOD.MERGE,
            key: `${ONYXKEYS.COLLECTION.SNAPSHOT}${hash}`,
            value: {
                search: {
                    ...(status && { status }),
                },
                data: undefined,
            },
        }
    ];

What alternative solutions did you explore? (Optional)

Or the backend should send the correct result in Search api.

Contributor details

Your Expensify account email: [email protected] Upwork Profile Link: https://www.upwork.com/freelancers/~01aff093c9a804b145

luckydaniel1993 avatar Nov 22 '24 09:11 luckydaniel1993

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

melvin-bot[bot] avatar Nov 22 '24 09:11 melvin-bot[bot]

Edited by proposal-police: This proposal was edited at 2024-11-29 04:40:23 UTC.

Proposal

Please re-state the problem that we are trying to solve in this issue.

Search loads infinitely when search query contains type:chat and category query

What is the root cause of that problem?

This issue occurred because we sent incorrect parameters to the Search API. On the backend, there is parameter validation that returns a 402 error code.

For example, if type === chat, we cannot set the key as category, expenseType, or currency,... because they are used for expense types. This is validated on the backend, or there is logic that does not handle this case, resulting in an error.

  • Screenshot 2024-11-25 at 15 27 07

So, the backend response onyxData is empty, causing the condition to hide the loading indicator to fail. As a result, the loading indicator remains visible when onyxData is empty, as shown in the response log above.

https://github.com/Expensify/App/blob/754a11c8dbc3532bb4201055c8bdceb4c75c674d/src/components/Search/index.tsx#L194-L199

For example, the backend returns a 402 error: type:chat status:all in:mountain tag:abcd type:chat status:all in:mountain expenseType:abcd type:chat status:all in:mountain currency:abcd

Because this query is only correct for type:expense

What changes do you think we should make in order to solve the problem?

To resolve this issue, we should check if type === chat, and in that case, don't accept category, expenseType, or currency ... as filter keys. Instead, convert them to quoted values, or if we want to change anything, we can discuss it in the pull request.. Something like this:

function buildSearchQueryString(queryJSON?: SearchQueryJSON) {
    ...
// src/libs/SearchQueryUtils.ts#L313
    for (const filter of filters) {
+        const includeQuotes =
+            queryParts.some((item) => item === 'type:chat') &&
+            (filter.key === FILTER_KEYS.CATEGORY ||
+                filter.key === FILTER_KEYS.EXPENSE_TYPE ||
+                filter.key === FILTER_KEYS.TAG ||
+                filter.key === FILTER_KEYS.TAX_RATE ||
+                filter.key === FILTER_KEYS.DESCRIPTION ||
+                filter.key === FILTER_KEYS.MERCHANT ||
+                filter.key === FILTER_KEYS.CARD_ID ||
+                filter.key === FILTER_KEYS.CURRENCY);

        const filterValueString = buildFilterValuesString(filter.key, filter.filters);

+        if (includeQuotes) {
+            queryParts.push(`"${filterValueString}"`.replace(/\s+/g, ''));
+        } else {
            queryParts.push(filterValueString);
+        }
    }

    return queryParts.join(' ');
}

Note: if other type (trip or ...) same with that case, then we able reuse this solution.

Test branch

POC

https://github.com/user-attachments/assets/b0cc8583-bb61-41ae-831d-7a75c913cc36

What alternative solutions did you explore? (Optional)

To resolve this issue, we need to do two things:

  1. Double-check the JSON query to match the backend's requirements.
  2. Handle the case where onyxData is an empty array.

https://github.com/Expensify/App/blob/9933888bd1576e2723560b938945b805b8a8d3d3/src/components/Search/index.tsx#L308

//src/components/Search/index.tsx#L308
+    if (searchResults === undefined && !isOffline) {
+        setShouldShowStatusBarLoading(false);

+        return (
+            <View style={[shouldUseNarrowLayout ? styles.searchListContentContainerStyles : styles.mt3, styles.flex1]}>
+                <EmptySearchView type={type} />
+            </View>
+        );
+    }
POC Screenshot 2024-11-29 at 11 37 23

huult avatar Nov 25 '24 08:11 huult

@Christinadobrzyn, @alitoshmatov Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] avatar Nov 25 '24 09:11 melvin-bot[bot]

@alitoshmatov can you review these proposals? Thanks!

Christinadobrzyn avatar Nov 25 '24 15:11 Christinadobrzyn

that make the condition shouldShowLoadingState is still false

@daledah I assume you meant isDataLoaded is still false, your RCA is correct. Can you elaborate how you want to show error. I like the idea of adding failureData to handle any errors in api.

alitoshmatov avatar Nov 26 '24 09:11 alitoshmatov

@Anaslancer Thank you for your proposal, your solution is the same as @daledah 's.

alitoshmatov avatar Nov 26 '24 09:11 alitoshmatov

@huult Thank you for detailed proposal, your RCA is correct. But I don't think your solution is future-proof, your solution does solve this issue, but when backend responds with another error the same thing happens again.

alitoshmatov avatar Nov 26 '24 09:11 alitoshmatov

@alitoshmatov Thank you for your feedback.

This issue occurred because the app sent a query with incorrect syntax to the backend, which the backend could not process, resulting in an error.

Therefore, I think we should validate the query to ensure the correct syntax is sent to the backend. With this approach, we can perform the search; if not, we can handle the error by hiding the loading indicator and showing a 'not found' message. However, this might introduce a new issue where the category is sent, but the search result is 'not found'.

But I don't think your solution is future-proof,

With this concern, I suggest we confirm with the internal team to get information about the filter key that the backend can support.

For example:

type = expense support key: category, tag... type = chat support key: from, to... type = trip support key: from, to...

to we can create the correct query syntax.

@alitoshmatov If you agree with this, we can discuss it in detail and define the next steps

huult avatar Nov 26 '24 14:11 huult

Proposal updated

  • Add alternative solutions

huult avatar Nov 29 '24 04:11 huult

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] avatar Nov 29 '24 16:11 melvin-bot[bot]

@alitoshmatov I think we can show the error at the end of search page same as what we did for form. We can get some thoughts from design team. What do you think?

daledah avatar Nov 30 '24 02:11 daledah

@Christinadobrzyn, @alitoshmatov Eep! 4 days overdue now. Issues have feelings too...

melvin-bot[bot] avatar Dec 02 '24 09:12 melvin-bot[bot]

@huult I still think validating query is partial solution, we can't possibly cover all cases where backend sends an error. Moreover this issue is rare since it requires user to type all filters by hand. I also got similar case where even with valid search query backend is responding with an empty data which is causing infinite loading

Screenshot 2024-12-02 at 4 06 56 PM Screenshot 2024-12-02 at 4 07 10 PM

alitoshmatov avatar Dec 02 '24 11:12 alitoshmatov

@daledah We are not receiving the error in onyx data, how are you going to access it?

alitoshmatov avatar Dec 02 '24 11:12 alitoshmatov

@alitoshmatov Yes, I have added alternative solutions to handle cases where the back end returns Onyx an empty array. Can you check it?

huult avatar Dec 02 '24 11:12 huult

@alitoshmatov We can get the error in https://github.com/Expensify/App/blob/b80c68ea76cdf825d9331bbf298cc41b40716037/src/components/Search/index.tsx#L134

currentSearchResults.error

daledah avatar Dec 03 '24 03:12 daledah

@daledah Are you sure, currentSearchResults.error contains error that backend sends inside onyxUpdate, right now there is no data inside onyx

Screenshot 2024-12-04 at 10 17 02 PM

alitoshmatov avatar Dec 04 '24 17:12 alitoshmatov

@alitoshmatov I think my alternative solution is suitable for this case. If the user inputs a wrong query and is unable to retrieve any data from onyxData, we should display an empty search view.

huult avatar Dec 04 '24 17:12 huult

@Christinadobrzyn @alitoshmatov this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

melvin-bot[bot] avatar Dec 05 '24 09:12 melvin-bot[bot]

Are you sure, currentSearchResults.error contains error that backend sends inside onyxUpdate, right now there is no data inside onyx

@alitoshmatov No I mean creating error on failureData

daledah avatar Dec 06 '24 03:12 daledah

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] avatar Dec 06 '24 16:12 melvin-bot[bot]

Okay, I think we can go with @daledah's proposal which suggest these:

  • Show empty page when request returns error
  • Update failureData with custom error to show to user

P. S. for showing error part, we do not have any similar case in search page so this is new thing, we probably need to confirm its design with others. Or another option is just not show error. We just show empty page when request returns error

C+ reviewed 🎀 👀 🎀

alitoshmatov avatar Dec 06 '24 21:12 alitoshmatov

Triggered auto assignment to @Julesssss, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

melvin-bot[bot] avatar Dec 06 '24 21:12 melvin-bot[bot]

Hey @luacmartins. Sorry to add to your work but we're making search-related changes when request error, and thought you should be notified.

Do you see any issues with the following for now?:

  • Show empty page when request returns error: Node passed as search filter is invalid for the reportActions table

Julesssss avatar Dec 09 '24 10:12 Julesssss