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

[$250] [Search v2.1] - App shows not here page when category with emoji is used in search filters #48627

Closed
6 tasks done
IuliiaHerets opened this issue Sep 5, 2024 · 31 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review

Comments

@IuliiaHerets
Copy link

IuliiaHerets commented Sep 5, 2024

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Version Number: 9.0.29-6
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers): applausetester+kh010901@applause.expensifail.com
Issue reported by: Applause Internal Team

Action Performed:

  1. Go to staging.new.expensify.com
  2. Go to workspace settings > Categories.
  3. Add a category with just an emoji.
  4. Go to Search.
  5. Click Filters.
  6. Click Categories.
  7. Select the category with emoji.
  8. Click Save.
  9. Click View results.

Expected Result:

App will not show not here page.

Actual Result:

App shows not here page when category with emoji is used in search filters.

Workaround:

Unknown

Platforms:

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Bug6592936_1725458348658.20240904_215707.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021833650551024180352
  • Upwork Job ID: 1833650551024180352
  • Last Price Increase: 2024-09-10
  • Automatic offers:
    • jaydamani | Contributor | 103944838
Issue OwnerCurrent Issue Owner: @Pujan92
@IuliiaHerets IuliiaHerets added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Sep 5, 2024
Copy link

melvin-bot bot commented Sep 5, 2024

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

@IuliiaHerets
Copy link
Author

We think that this bug might be related to #wave-control

@IuliiaHerets
Copy link
Author

@strepanier03 FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

@NJ-2020
Copy link
Contributor

NJ-2020 commented Sep 5, 2024

Proposal

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

Search filters - App shows not here page when category with emoji is used in search filters

What is the root cause of that problem?

Inside the searchParser regex, we do not include for emojis

var peg$r2 = /^[A-Za-z0-9_@.\/#&+\-\\',;]/;

var peg$e29 = peg$classExpectation([["A", "Z"], ["a", "z"], ["0", "9"], "_", "@", ".", "/", "#", "&", "+", "-", "\\", "'", ",", ";"], false, false);

So the regex match return value of null instead of this:
Screenshot 2024-09-05 at 02 39 52

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

We can include the emojis regex by adding this

\u00a9\u00ae\u2000-\u3300\ud83c\ud000-\udfff\ud83d\ud000-\udfff\ud83e\ud000-\udfff

So it will be:

var peg$r2 = /^[A-Za-z0-9_@.\/#&+\-\\',;\u00a9\u00ae\u2000-\u3300\ud83c\ud000-\udfff\ud83d\ud000-\udfff\ud83e\ud000-\udfff]/;
....
var peg$e29 = peg$classExpectation([["A", "Z"], ["a", "z"], ["0", "9"], "_", "@", ".", "/", "#", "&", "+", "-", "\\", "'", ",", ";", "\u00a9","\u00ae", ["\u2000", "\u3300"], ["\ud83c", "\udfff"], ["\ud83d", "\udfff"], ["\ud83e", "\udfff"]], false, false);

Result

Screen.Recording.2024-09-05.at.02.44.31.mov

What alternative solutions did you explore? (Optional)

@jaydamani
Copy link
Contributor

jaydamani commented Sep 5, 2024

Edited by proposal-police: This proposal was edited at 2024-09-05 11:13:47 UTC.

Proposal

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

search filter does not work when category contains emoji

What is the root cause of that problem?

Based on current grammar from SearchParser.peggy, filters need to be alphanumeric (with some extra special symbols), non alphanumeric strings need to be wrapped around quotes so, category:test😃 is invalid but category:"test😃" is valid.

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

Similar thing can happen for space but for that, the quotes are added to query string by SearchUtils.sanitizeString so, we need to update SearchUtils.sanitizeString to add quotes whenever the string contains non alphanumeric character by checking that it matches /[^A-Za-z0-9_@./#&+\-\\',;]/

What alternative solutions did you explore? (Optional)

Update definition of alphanumeric to include emojis. It would look something like [A-Za-z0-9_@./#&+\-\\',;\u00a9\u00ae].

@jaydamani
Copy link
Contributor

Updated proposal

  • added a better solution

@melvin-bot melvin-bot bot added the Overdue label Sep 9, 2024
Copy link

melvin-bot bot commented Sep 10, 2024

@strepanier03 Eep! 4 days overdue now. Issues have feelings too...

@strepanier03

This comment was marked as outdated.

@melvin-bot melvin-bot bot removed the Overdue label Sep 10, 2024
@strepanier03 strepanier03 added the External Added to denote the issue can be worked on by a contributor label Sep 10, 2024
@melvin-bot melvin-bot bot changed the title Search filters - App shows not here page when category with emoji is used in search filters [$250] Search filters - App shows not here page when category with emoji is used in search filters Sep 10, 2024
Copy link

melvin-bot bot commented Sep 10, 2024

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

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

melvin-bot bot commented Sep 10, 2024

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

@Pujan92
Copy link
Contributor

Pujan92 commented Sep 12, 2024

@NJ-2020 searchParser.js file content is generated via searchParser.peggy file and we are not supposed to edit directly in searchParser.js.

I like the @jaydamani's proposal where we can wrap the string with double quotes for non-alphanumeric case. Also tagging @289Adam289 for a quick look and confirmation as they have worked on it.

🎀👀🎀 C+ reviewed

Copy link

melvin-bot bot commented Sep 12, 2024

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

@aldo-expensify
Copy link
Contributor

aldo-expensify commented Sep 12, 2024

@luacmartins can I get your eyes on the proposals mentioned. The result in slightly different behaviour, so I would like to confirm which one we want from a user point of view too.

My understanding is that:

  • @jaydamani will end up wrapping in double quotes the text if it contains non-alphanumeric characters
  • @NJ-2020 doesn't end up wrapping in double quotes.

@jaydamani introduction of double quotes is fine for me, since we already do that when there are spaces, but wanted to confirm.

@luacmartins
Copy link
Contributor

@jaydamani's proposal seems good. We can go with it. Tagging @Kicu @289Adam289 @Guccio163 @SzymczakJ for visibility

@289Adam289
Copy link
Contributor

289Adam289 commented Sep 13, 2024

@jaydamani's proposal of wrapping every string that includes character that does match our definition of alphanumeric is an ok solution. It will also solve this issue #49114. It isn't perfect but given that we have encountered similar issues (#46993, #48627, #48407) before I would go with.

One thing I'd like to point out is that this problem will appear in the new Search Router Input #49119 where we cannot wrap strings in quote marks and thus we might have to improve search parsers.

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

melvin-bot bot commented Sep 13, 2024

📣 @jaydamani 🎉 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 📖

@luacmartins
Copy link
Contributor

@jaydamani please open a PR for this. Also please link both #48627 and #49114 in the list of solved issues, since your solution will also solve the 2nd issue.

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Sep 15, 2024
@jaydamani
Copy link
Contributor

@Pujan92 The PR is ready to review.

@luacmartins luacmartins changed the title [$250] Search filters - App shows not here page when category with emoji is used in search filters [$250] [Search v2.1] - App shows not here page when category with emoji is used in search filters Sep 19, 2024
@jaydamani
Copy link
Contributor

jaydamani commented Sep 24, 2024

Deployed to production. Payment is due tomorrow.

@roryabraham roryabraham added Bug Something is broken. Auto assigns a BugZero manager. and removed Bug Something is broken. Auto assigns a BugZero manager. labels Sep 26, 2024
Copy link

melvin-bot bot commented Sep 26, 2024

Triggered auto assignment to @zanyrenney (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 melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Sep 26, 2024
@zanyrenney
Copy link
Contributor

payment summary

paid @jaydamani $250 via upwork

@zanyrenney
Copy link
Contributor

@Pujan92 looks like you didn't apply to the job? can you do that please?

@zanyrenney
Copy link
Contributor

Just an FYI I'll be OOO until Tuesday 1st, so I am reapplying the bug label. Last step is to pay @Pujan92 -- Otherwise I will get to it when I return. Thanks!

@zanyrenney zanyrenney added Bug Something is broken. Auto assigns a BugZero manager. and removed Bug Something is broken. Auto assigns a BugZero manager. labels Sep 26, 2024
@zanyrenney zanyrenney removed their assignment Sep 26, 2024
Copy link

melvin-bot bot commented Sep 26, 2024

Current assignee @zanyrenney is eligible for the Bug assigner, not assigning anyone new.

@zanyrenney zanyrenney added Bug Something is broken. Auto assigns a BugZero manager. and removed Bug Something is broken. Auto assigns a BugZero manager. labels Sep 26, 2024
Copy link

melvin-bot bot commented Sep 26, 2024

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

@zanyrenney
Copy link
Contributor

Hey @JmillsExpensify please can you pay out @Pujan92 when they apply to the job! Full details here

Thanks!

@Pujan92
Copy link
Contributor

Pujan92 commented Sep 26, 2024

@zanyrenney I will request in NewDot. Can you Plz add in in payment summary.

@zanyrenney
Copy link
Contributor

payment summary

paid @jaydamani $250 via upwork
@Pujan92 please request 250 on ND.

@garrettmknight
Copy link
Contributor

$250 approved for @Pujan92

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. Daily KSv2 External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review
Projects
Status: Done
Development

No branches or pull requests