Skip to content
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

[HOLD for payment 2025-01-02] [$250] Search - Search loads infinitely when search query contains type:chat and category query #52907

Open
8 tasks done
IuliiaHerets opened this issue Nov 21, 2024 · 47 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Weekly KSv2

Comments

@IuliiaHerets
Copy link

IuliiaHerets commented Nov 21, 2024

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:

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

Screenshots/Videos

Bug6671890_1732196532024.20241121_212559.mp4

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: @Christinadobrzyn
@IuliiaHerets IuliiaHerets added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Nov 21, 2024
Copy link

melvin-bot bot commented Nov 21, 2024

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.

@daledah
Copy link
Contributor

daledah commented Nov 21, 2024

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

isLoading: false,

that make the condition shouldShowLoadingState is still false

const isDataLoaded =
searchResults?.data !== undefined && searchResults?.search?.type === type && Array.isArray(status)
? searchResults?.search?.status === status.join(',')
: searchResults?.search?.status === status;
const shouldShowLoadingState = !isOffline && !isDataLoaded;

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

return {optimisticData, finallyData};

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

@Christinadobrzyn
Copy link
Contributor

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 Christinadobrzyn added the External Added to denote the issue can be worked on by a contributor label Nov 22, 2024
@melvin-bot melvin-bot bot changed the title Search - Search loads infinitely when search query contains type:chat and category query [$250] Search - Search loads infinitely when search query contains type:chat and category query Nov 22, 2024
Copy link

melvin-bot bot commented Nov 22, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Nov 22, 2024
Copy link

melvin-bot bot commented Nov 22, 2024

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

@Anaslancer
Copy link
Contributor

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

const isDataLoaded =
searchResults?.data !== undefined && searchResults?.search?.type === type && Array.isArray(status)
? searchResults?.search?.status === status.join(',')
: searchResults?.search?.status === status;
const shouldShowLoadingState = !isOffline && !isDataLoaded;

shouldShowLoadingState is true because searchResults?.search?.status is undefined.
According to this below logic, we are showing the infinite loading component.
if (shouldShowLoadingState) {
return (
<SearchRowSkeleton
shouldAnimate
containerStyle={shouldUseNarrowLayout && styles.searchListContentContainerStyles}
/>
);
}

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

Copy link

melvin-bot bot commented Nov 22, 2024

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

@huult
Copy link
Contributor

huult commented Nov 25, 2024

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.

const isDataLoaded =
searchResults?.data !== undefined && searchResults?.search?.type === type && Array.isArray(status)
? searchResults?.search?.status === status.join(',')
: searchResults?.search?.status === status;
const shouldShowLoadingState = !isOffline && !isDataLoaded;

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
Screen.Recording.2024-11-25.at.15.43.11.mov

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.

//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

@melvin-bot melvin-bot bot added the Overdue label Nov 25, 2024
Copy link

melvin-bot bot commented Nov 25, 2024

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

@Christinadobrzyn
Copy link
Contributor

@alitoshmatov can you review these proposals? Thanks!

@alitoshmatov
Copy link
Contributor

alitoshmatov commented Nov 26, 2024

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.

@melvin-bot melvin-bot bot removed the Overdue label Nov 26, 2024
@alitoshmatov
Copy link
Contributor

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

@alitoshmatov
Copy link
Contributor

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

@huult
Copy link
Contributor

huult commented Nov 26, 2024

@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

@melvin-bot melvin-bot bot added the Overdue label Nov 29, 2024
@huult
Copy link
Contributor

huult commented Nov 29, 2024

Proposal updated

  • Add alternative solutions

Copy link

melvin-bot bot commented Nov 29, 2024

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

@muttmuure muttmuure moved this to MEDIUM in [#whatsnext] #quality Nov 29, 2024
@daledah
Copy link
Contributor

daledah commented Nov 30, 2024

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

Copy link

melvin-bot bot commented Dec 2, 2024

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

@melvin-bot melvin-bot bot added the Overdue label Dec 9, 2024
@luacmartins
Copy link
Contributor

I think that's fine, but maybe we want to display a no results found page. cc @Expensify/design for ideas

@Christinadobrzyn
Copy link
Contributor

Christinadobrzyn commented Dec 9, 2024

@Julesssss, @luacmartins, @alitoshmatov

Just to understand, we're thinking this is the flow?

  • Go to staging.new.expensify.com
  • Click search router icon.
  • Enter the following query: type:chat in: category:car
  • Hit Enter.
  • Error returned no results found - We'd like @Expensify/design to provide feedback on this error. Is that right or is there another error we're showing during these steps?

@shawnborton
Copy link
Contributor

Yeah, I think based on what I am seeing above, I would expect us to just show a "Nothing to show" page, like this:
CleanShot 2024-12-09 at 15 11 39@2x

@dannymcclain
Copy link
Contributor

Yeah I agree. I might advocate for updating that copy to something like "Try adjusting your search criteria or creating something with the green + button." for all of these nothing found screens though.

@Christinadobrzyn
Copy link
Contributor

Amazing! Thanks @shawnborton and @dannymcclain! Jules does this sound good to move forward with #52907 (comment)

@melvin-bot melvin-bot bot removed the Overdue label Dec 9, 2024
@dubielzyk-expensify
Copy link
Contributor

Love the mock and great suggestion Danny 👍

@Julesssss
Copy link
Contributor

@Christinadobrzyn yeah, this sounds good to me. Assigning now.

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Dec 10, 2024
Copy link

melvin-bot bot commented Dec 10, 2024

📣 @daledah 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@alitoshmatov
Copy link
Contributor

alitoshmatov commented Dec 10, 2024

Yeah I agree. I might advocate for updating that copy to something like "Try adjusting your search criteria or creating something with the green + button." for all of these nothing found screens though.

Several people 👍 this so I assume we are also implementing it.

@daledah Please update Try creating something with the green + button to Try adjusting your search criteria or try creating something with the green + button.

Please respond if copy should be something different

@Christinadobrzyn
Copy link
Contributor

Just a heads up that I'm going to be ooo Dec 12 - 13th. Back on Monday 16th. I'm not going to assign this to a BZ teammate but if anything is urgent, please reach out to the team for a volunteer.

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Dec 13, 2024
@mvtglobally
Copy link

Issue not reproducible during KI retests. (First week)

@alitoshmatov
Copy link
Contributor

Issue not reproducible during KI retests. (First week)

Issue is still reproducable with type:chat status:unread in:Mountain category:car query

@Christinadobrzyn
Copy link
Contributor

hi @alitoshmatov and @daledah just checking on the PR for this. Can you provide an update?

@alitoshmatov
Copy link
Contributor

PR in progress, waiting @daledah to apply changes to text copies based on slack conversation here

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Dec 26, 2024
@melvin-bot melvin-bot bot changed the title [$250] Search - Search loads infinitely when search query contains type:chat and category query [HOLD for payment 2025-01-02] [$250] Search - Search loads infinitely when search query contains type:chat and category query Dec 26, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Dec 26, 2024
Copy link

melvin-bot bot commented Dec 26, 2024

Reviewing label has been removed, please complete the "BugZero Checklist".

Copy link

melvin-bot bot commented Dec 26, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.78-6 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2025-01-02. 🎊

For reference, here are some details about the assignees on this issue:

Copy link

melvin-bot bot commented Dec 26, 2024

@alitoshmatov @Christinadobrzyn @alitoshmatov The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Weekly KSv2
Projects
Development

No branches or pull requests