-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Comments
Triggered auto assignment to @strepanier03 ( |
We think that this bug might be related to #wave-control |
@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 |
ProposalPlease 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 App/src/libs/SearchParser/searchParser.js Line 208 in 8a35256
App/src/libs/SearchParser/searchParser.js Line 240 in 8a35256
So the regex match return value of null instead of this: 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); ResultScreen.Recording.2024-09-05.at.02.44.31.movWhat alternative solutions did you explore? (Optional) |
Edited by proposal-police: This proposal was edited at 2024-09-05 11:13:47 UTC. ProposalPlease 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, 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 What alternative solutions did you explore? (Optional)Update definition of alphanumeric to include emojis. It would look something like |
Updated proposal
|
@strepanier03 Eep! 4 days overdue now. Issues have feelings too... |
This comment was marked as outdated.
This comment was marked as outdated.
Job added to Upwork: https://www.upwork.com/jobs/~021833650551024180352 |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @Pujan92 ( |
@NJ-2020 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 |
Triggered auto assignment to @aldo-expensify, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
@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 introduction of double quotes is fine for me, since we already do that when there are spaces, but wanted to confirm. |
@jaydamani's proposal seems good. We can go with it. Tagging @Kicu @289Adam289 @Guccio163 @SzymczakJ for visibility |
@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. |
📣 @jaydamani 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app! Offer link |
@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. |
Deployed to production. Payment is due tomorrow. |
Triggered auto assignment to @zanyrenney ( |
payment summary paid @jaydamani $250 via upwork |
@Pujan92 looks like you didn't apply to the job? can you do that please? |
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! |
Current assignee @zanyrenney is eligible for the Bug assigner, not assigning anyone new. |
Triggered auto assignment to @JmillsExpensify ( |
Hey @JmillsExpensify please can you pay out @Pujan92 when they apply to the job! Full details here Thanks! |
@zanyrenney I will request in NewDot. Can you Plz add in in payment summary. |
payment summary paid @jaydamani $250 via upwork |
$250 approved for @Pujan92 |
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:
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:
Screenshots/Videos
Bug6592936_1725458348658.20240904_215707.mp4
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @Pujan92The text was updated successfully, but these errors were encountered: