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][$250] Search - Suggestion still shows the report when it is already selected as search query #52213

Closed
6 of 8 tasks
IuliiaHerets opened this issue Nov 7, 2024 · 39 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors Weekly KSv2

Comments

@IuliiaHerets
Copy link

IuliiaHerets commented Nov 7, 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.59-0
Reproducible in staging?: Y
Reproducible in production?: N
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. Open any report.
  3. Open search router.
  4. Enter in: followed by report name.
  5. Select the report from the suggestion.

Expected Result:

The suggestion will not show the selected report because the report is already selected (production behavior).

Actual Result:

The suggestion shows the selected report when the report is already selected.
This issue only happens with in: search query and does not happen with from, to, category, tag etc

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Bug6657948_1731003896136.20241108_022133.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021854651066569405634
  • Upwork Job ID: 1854651066569405634
  • Last Price Increase: 2024-11-14
@IuliiaHerets IuliiaHerets added DeployBlockerCash This issue or pull request should block deployment Bug Something is broken. Auto assigns a BugZero manager. labels Nov 7, 2024
Copy link

melvin-bot bot commented Nov 7, 2024

Triggered auto assignment to @johncschuster (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.

Copy link

melvin-bot bot commented Nov 7, 2024

Triggered auto assignment to @MarioExpensify (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

@melvin-bot melvin-bot bot added the Daily KSv2 label Nov 7, 2024
Copy link

melvin-bot bot commented Nov 7, 2024

💬 A slack conversation has been started in #expensify-open-source

@github-actions github-actions bot added Engineering Hourly KSv2 and removed Daily KSv2 labels Nov 7, 2024
Copy link
Contributor

github-actions bot commented Nov 7, 2024

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

@nkdengineer
Copy link
Contributor

nkdengineer commented Nov 7, 2024

Edited by proposal-police: This proposal was edited at 2024-11-07 20:07:36 UTC.

Proposal

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

The suggestion shows the selected report when the report is already selected.
This issue only happens with in: search query and does not happen with from, to, category, tag etc

What is the root cause of that problem?

We are missing the filter in type in with alreadyAutocompletedKeys value similar to what we did with other types

const filteredChats = searchOptions.recentReports
.filter((chat) => chat.text?.toLowerCase()?.includes(autocompleteValue.toLowerCase()))
.sort((chatA, chatB) => (chatA > chatB ? 1 : -1))
.slice(0, 10);

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

We should add condition with alreadyAutocompletedKeys value in filter function here

const filteredChats = searchOptions.recentReports
      .filter((chat) => chat.text?.toLowerCase()?.includes(autocompleteValue.toLowerCase()) && !alreadyAutocompletedKeys.includes(chat.text?.toLowerCase()))
      .sort((chatA, chatB) => (chatA > chatB ? 1 : -1))
      .slice(0, 10);

We can find any missing types and do the same.

What alternative solutions did you explore? (Optional)

NA

@MarioExpensify
Copy link
Contributor

I was able to reproduce here, but hardly a deploy blocker. Let's make this External and move forward with the deploy.

@MarioExpensify MarioExpensify added Daily KSv2 External Added to denote the issue can be worked on by a contributor and removed DeployBlockerCash This issue or pull request should block deployment Hourly KSv2 labels Nov 7, 2024
@melvin-bot melvin-bot bot changed the title Search - Suggestion still shows the report when it is already selected as search query [$250] Search - Suggestion still shows the report when it is already selected as search query Nov 7, 2024
Copy link

melvin-bot bot commented Nov 7, 2024

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

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

melvin-bot bot commented Nov 7, 2024

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

@mohit6789
Copy link

mohit6789 commented Nov 8, 2024

Edited by proposal-police: This proposal was edited at 2024-11-08 06:41:39 UTC.

Proposal

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

Selected search item does not disappear from the suggestions list when using "in:" search query. Expected behavior works correctly for other types of queries.

What is the root cause of that problem?

In case of "in:" search query, we have missed to filter the suggestion list(!alreadyAutocompletedKeys.includes(chat.text.toLowerCase())) if it is already selected which already done for other types of search queries.

 const filteredChats = searchOptions.recentReports
       .filter((chat) => chat.text?.toLowerCase()?.includes(autocompleteValue.toLowerCase()))

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

The simplest fix would be to filter the autocomplete list by adding !alreadyAutocompletedKeys.includes(chat.text.toLowerCase()), as we’ve done in other places. However, I suggest an alternative, more scalable solution that will be beneficial in the long term.

const filteredChats = searchOptions.recentReports
         .filter((chat) => chat.text?.toLowerCase()?.includes(autocompleteValue.toLowerCase()) && !alreadyAutocompletedKeys.includes(chat.text.toLowerCase()))

What alternative solutions did you explore? (Optional)

The filtering logic for all search query types is currently the same. I suggest extracting this logic into a reusable function. This way, if a new query type is added or the filtering logic changes in the future, updates can be made in a single place, applying uniformly to all search types (e.g., "in:", "to:", "from:"). This will help prevent bugs like the current issue, where conditions may be missed.

function getFilteredAutoCompleteList(value: string) {
       return value?.includes(autocompleteValue.toLowerCase()) && !alreadyAutocompletedKeys.includes(value);
}

Copy link

melvin-bot bot commented Nov 8, 2024

📣 @mohit6789! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

  1. Make sure you've read and understood the contributing guidelines.
  2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
    Screen Shot 2022-11-16 at 4 42 54 PM
    Format:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

@mohit6789
Copy link

Contributor details
Your Expensify account email: [email protected]
Upwork Profile Link: Upwork

Copy link

melvin-bot bot commented Nov 8, 2024

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

@ntdiary
Copy link
Contributor

ntdiary commented Nov 8, 2024

I’ve reviewed proposals and left a comment here to avoid any conflicts. :)

@ntdiary
Copy link
Contributor

ntdiary commented Nov 20, 2024

@ntdiary This issue has been marked as External so I think the contributor's solution should be prioritized here. Could you please take a look at the contributors' proposals?

Hi, @nkdengineer. The reason I asked in PR #51633 is that this issue is introduced by that PR, and they still plan to make a follow-up PR to address some problems. Considering they are likely to refactor the related query logic (possibly even overriding your solution here), I felt at the time that it would be better to notify them and let them handle it. However, if their new PR adopts your solution, I personally think it would be fine to offer you some compensation. Would you be okay with that? :)

cc @johncschuster @MarioExpensify to see if there are any different thoughts.

BTW, the follow up PR is #52568.

@melvin-bot melvin-bot bot removed the Overdue label Nov 20, 2024
@johncschuster johncschuster moved this to Bugs and Follow Up Issues in [#whatsnext] #expense Nov 22, 2024
@melvin-bot melvin-bot bot added the Overdue label Nov 25, 2024
@johncschuster
Copy link
Contributor

This is on hold

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Nov 25, 2024
@MarioExpensify
Copy link
Contributor

On hold, not overdue! Moving this to Weekly so it stop being tagged as overdue CC @johncschuster

@MarioExpensify MarioExpensify added the Weekly KSv2 label Nov 28, 2024
@melvin-bot melvin-bot bot removed the Overdue label Nov 28, 2024
@MarioExpensify MarioExpensify removed the Daily KSv2 label Nov 28, 2024
@ntdiary
Copy link
Contributor

ntdiary commented Dec 3, 2024

PR #52568 has already been deployed to production 3 days ago, and they did adopt the solution from @nkdengineer that I mentioned in the comment, so this issue has been fixed. :)

image
test.mp4

@melvin-bot melvin-bot bot added the Overdue label Dec 11, 2024
@johncschuster
Copy link
Contributor

Thanks for your patience on this one! I've reviewed the internal payment docs, and it looks like @nkdengineer should be compensated for the solution that we wound up using.

I'll create an Upwork job for that now.

@melvin-bot melvin-bot bot removed the Overdue label Dec 11, 2024
@johncschuster
Copy link
Contributor

Job is here

@ntdiary
Copy link
Contributor

ntdiary commented Dec 12, 2024

Hi, @johncschuster, curious does c+ have any compensation here, for reviewing and picking proposal? :D

@johncschuster
Copy link
Contributor

Great question, @ntdiary! I was so focused on the "solution" that I didn't check for that in our docs. Let me check and come back to you on that.

@johncschuster
Copy link
Contributor

@ntdiary thanks for your patience! Yes, I will issue the same 50% to you as I would to @nkdengineer for their work on this issue. I've sent you an invite to the job. Please ping me when you've accepted that so I can pay this out!

@ntdiary
Copy link
Contributor

ntdiary commented Dec 14, 2024

@johncschuster, haha, I have switched to ND for payment a few months ago, so it's fine to just make a payment summary here. :D

@JmillsExpensify
Copy link

Yes, I only need he payment summary to approve the ND payment.

@johncschuster
Copy link
Contributor

Oh darn it. Thanks for calling that out, @ntdiary! Summary incoming.

@johncschuster
Copy link
Contributor

johncschuster commented Dec 16, 2024

Payment Summary:

Contributor: @nkdengineer paid $125 via Upwork

Contributor+: @ntdiary due $125 via NewDot

Upwork job here! Please apply

@johncschuster
Copy link
Contributor

@nkdengineer can you please accept the Upwork offer above so I can pay this out before I head out on holiday OOO?

@nkdengineer
Copy link
Contributor

@johncschuster I did. Enjoy your break! 🍷

@JmillsExpensify
Copy link

$125 approved for @ntdiary

@johncschuster
Copy link
Contributor

Payment has been issued. Thanks, everyone!

@github-project-automation github-project-automation bot moved this from Bugs and Follow Up Issues to Done in [#whatsnext] #expense Dec 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors Weekly KSv2
Projects
Status: Done
Development

No branches or pull requests

7 participants